[webkit-reviews] review denied: [Bug 208265] Make Path::Path(const Path&) and Path::operator=(const Path&) cheap : [Attachment 392074] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 208265: Make Path::Path(const Path&) and Path::operator=(const Path&) cheap
https://bugs.webkit.org/show_bug.cgi?id=208265

Attachment 392074: Patch

https://bugs.webkit.org/attachment.cgi?id=392074&action=review




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 392074
  --> https://bugs.webkit.org/attachment.cgi?id=392074
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392074&action=review

Looks like a great idea. I’m going to say review- because I spotted some errors
that need to be fixed and I’d like to review the fixed version.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:101
> +void Path::copyPathBeforeMutationIfNeeded()

We don’t need to add a new function for this. Every call added is right before
a call to ensurePlatformPlath, except for Path::transform (see below). While
the name doesn’t make this clear, ensurePlatformPath is *always* called just
before modifying the path. So this code could be moved inside that function;
maybe you want to rename it?

> Source/WebCore/platform/graphics/cg/PathCG.cpp:108
> +    if (m_path) {

Should not need this if statement. The rest of the code enforces the invariant
that m_copyPathBeforeMutation is only true if m_path is non-null.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:110
> +	   CGPathRelease(m_path);
> +	   m_path = CGPathCreateMutableCopy(m_path);

It’s *critical* not to release the old path before making the mutable copy.
It’s possible that the other owner is no longer retaining the path and so that
could be releasing the very last reference to it.

This points to another possible optimization, less important but we could do
it. If the reference count happens to be 1, then we got lucky and don’t need to
make a mutable copy. CFGetRetainCount can be used to check if it’s 1. Although
we use CGPathRetain and CGPathRelease we can use
CFRetain/CFRelease/CFGetRetainCount.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:125
> +    m_path = other.m_path;

I’d prefer construction style syntax for this, rather than assignment.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:133
>  Path::Path(Path&& other)

This move constructor needs to be updated; it’s unsafe as-is. When we take the
path from "other" we also need to take the m_copyPathBeforeMutation value from
"other" and set it to false in other. I’d write it like this:

    Path::Path(Path&& other)
	: m_path(std::exchange(other.m_path, nullptr))
	,
m_copyPathBeforeMutation(std::exchange(other.m_copyPathBeforeMutation, false))
    {
    }

> Source/WebCore/platform/graphics/cg/PathCG.cpp:150
> -    m_path = other.m_path ? CGPathCreateMutableCopy(other.m_path) : nullptr;
> +    m_path = other.m_path;
> +    if (m_path) {
> +	   m_copyPathBeforeMutation = true;
> +	   other.m_copyPathBeforeMutation = true;
> +	   CGPathRetain(m_path);
> +    }

This function is starting to get a bit long. There’s a great C++ idiom for
avoiding this and correctly writing an assignment operator no matter how
complex the rules are, once you’ve written a correct copy constructor. These
lines will do the job:

    Path copy { other };
    std::swap(m_path, copy.m_path);
    std::swap(m_copyPathBeforeMutation, copy.m_copyPathBeforeMutation);
    return *this;

This takes advantage of the copy constructor and the destructor.

The move assignment operator below makes the same mistake as the move
constructor above: it leaves the m_copyPathBeforeMutation boolean behind on the
other object instead of moving it with the path. Using this technique we can
also correct the move assignment operator in a nice way. We just use the the
same code as above, except for the first line:

    Path copy { WTFMove(other) };
    std::swap(m_path, copy.m_path);
    std::swap(m_copyPathBeforeMutation, copy.m_copyPathBeforeMutation);
    return *this;

Can also make both of these a bit more elegant by writing a separate swap
function that swaps the two data members. If we write that then the two
std::swap lines look like this:

    swap(*this, copy);

or this:

    swap(copy);

> Source/WebCore/platform/graphics/cg/PathCG.cpp:243
> +    copyPathBeforeMutationIfNeeded();

This makes a wasteful copy. The code below uses m_path to make a new path and
then releases it. There’s no reason to make a mutable copy just to do that. So
this call should be omitted.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:330
> +    copyPathBeforeMutationIfNeeded();

Could move this closer to before each of the two CGPath function calls here.
It’s not needed when we call addBeziersForRoundedRect, and I think it’s easier
to understand if we do it right before the function calls.