Bug 208265 – Make Path::Path(const Path&) and Path::operator=(const Path&) cheap

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

We can make these behave in a more "copy-on-write” manner, such that we just set m_path when copying a new WebCore::Path, and only call CGPathCreateMutableCopy later if it is needed.

[Wenson Hsieh]

Created attachment 392046 [details]
For EWS

[Radar WebKit Bug Importer]

<rdar://problem/59922452>

[Wenson Hsieh]

Created attachment 392074 [details]
Patch

[Darin Adler]

Comment on attachment 392074 [details]
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.

[Wenson Hsieh]

Comment on attachment 392074 [details]
Patch

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

>> 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?

This sounds good, but I’m not sure what I would rename ensurePlatformPath to since I don’t think it’s good for the details of this optimization to leak into all the call sites of ensurePlatformPath().

Maybe something like:
- Rename ensurePlatformPath() to ensureOrCopyPlatformPathIfNeeded() and make it private.
- Add a public ensurePlatformPath() method that just calls ensureOrCopyPlatformPathIfNeeded().

Does that sound reasonable?

>> 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.

Good catch.

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:110
>> +        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.

I think I might be missing something here — without the CGPathRelease here, we’d have a memory leak when a path is copied, and then both the original and copy are mutated. i.e.:

1. We have a Path a with a +1 CGPathRef p.
2. We copy a to another Path b, which +1's CGPathRef p.
3. We mutate a, which changes a’s CGPathRef to a new copy, q.
4. a is then destroyed, which releases q. q’s retain count is 0 and it is freed.
5. b is then destroyed, which releases p. p’s retain count is 1 and it is still alive.

…whereas if we release here, then p would be destroyed in (5) because it is additionally released in (3).

I will add a check to avoid copying when the retain count is 1.

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:125
>> +    m_path = other.m_path;
> 
> I’d prefer construction style syntax for this, rather than assignment.

Ok, changed to use construction style syntax.

>> 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))
>     {
>     }

Fixed!

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:150
>> +    }
> 
> 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);

Thanks! I’ve added a swap method and changed the implementations of the assignment operators to use it.

>> 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.

Good point! Removed this.

>> 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.

I will address this by moving the logic currently in copyPathBeforeMutationIfNeeded() into what is currently called ensurePlatformPath().

[Darin Adler]

Comment on attachment 392074 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cg/PathCG.cpp:110
>>> +        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.
> 
> I think I might be missing something here — without the CGPathRelease here, we’d have a memory leak when a path is copied, and then both the original and copy are mutated. i.e.:
> 
> 1. We have a Path a with a +1 CGPathRef p.
> 2. We copy a to another Path b, which +1's CGPathRef p.
> 3. We mutate a, which changes a’s CGPathRef to a new copy, q.
> 4. a is then destroyed, which releases q. q’s retain count is 0 and it is freed.
> 5. b is then destroyed, which releases p. p’s retain count is 1 and it is still alive.
> 
> …whereas if we release here, then p would be destroyed in (5) because it is additionally released in (3).
> 
> I will add a check to avoid copying when the retain count is 1.

My point was about the *order* of operations. The code I was reviewing could *destroy* the path by calling CGPathRelease and then call CGPathCreateMutableCopy on the destroyed path. Need to copy *before* releasing in case the retain count is 1. But if you special case the retain count of 1 that is one way of taking care of it.

[Wenson Hsieh]

Comment on attachment 392074 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/cg/PathCG.cpp:110
>>>> +        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.
>> 
>> I think I might be missing something here — without the CGPathRelease here, we’d have a memory leak when a path is copied, and then both the original and copy are mutated. i.e.:
>> 
>> 1. We have a Path a with a +1 CGPathRef p.
>> 2. We copy a to another Path b, which +1's CGPathRef p.
>> 3. We mutate a, which changes a’s CGPathRef to a new copy, q.
>> 4. a is then destroyed, which releases q. q’s retain count is 0 and it is freed.
>> 5. b is then destroyed, which releases p. p’s retain count is 1 and it is still alive.
>> 
>> …whereas if we release here, then p would be destroyed in (5) because it is additionally released in (3).
>> 
>> I will add a check to avoid copying when the retain count is 1.
> 
> My point was about the *order* of operations. The code I was reviewing could *destroy* the path by calling CGPathRelease and then call CGPathCreateMutableCopy on the destroyed path. Need to copy *before* releasing in case the retain count is 1. But if you special case the retain count of 1 that is one way of taking care of it.

I understand now — thanks for the clarification!

The CFGetRetainCount check indeed mitigates this, but I've still shuffled this code around so that we destroy the previous CGPath after copying it.

[Darin Adler]

Comment on attachment 392074 [details]
Patch

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

>>> 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?
> 
> This sounds good, but I’m not sure what I would rename ensurePlatformPath to since I don’t think it’s good for the details of this optimization to leak into all the call sites of ensurePlatformPath().
> 
> Maybe something like:
> - Rename ensurePlatformPath() to ensureOrCopyPlatformPathIfNeeded() and make it private.
> - Add a public ensurePlatformPath() method that just calls ensureOrCopyPlatformPathIfNeeded().
> 
> Does that sound reasonable?

Sounds good. The key is to keep this function name clear and short — a really long name would be irritating. I would be tempted to call it something like mutablePath() rather than using name more like "get ready to modify platform path by copying if needed and return the pointer".

I didn’t realize there was a call to ensurePlatformPath for a purpose other than modifying the path. I now see the *single* call outside of PathCG.cpp in the iOS DragDropInteractionState::previewForDragItem, which I had overlooked. All other callers with similar uses call platformPath() and either know it's not empty or check isEmpty(). Would be nice to separate this from all these uses for modifying the path inside the CG path implementation.

I now see a design problem in the Path class that we should eventually fix. The "PlatformPathPtr" abstraction is not helping. It makes it impossible to use RetainPtr in Path, but with no abstraction benefit that I can see. For the Cocoa versions of WebKit at least, there are no calls to the platformPath or ensurePlatformPath functions in platform-independent code at all. If we used RetainPtr, it would be a lot harder to get the retain/release wrong in the PathCG implementation. Eventually we would want something like this:

    public:
#if USE(CG)
        CGPathRef> pathCG() const;
        <or>
        RetainPtr<CGPathRef> pathCG() const; // <<< no copy in the name because this just retains for object lifetime safety, doesn’t copy for shared mutability safety >>>
#endif

    private:
#if USE(CG)
        CGMutablePathRef mutablePath();

        mutable RetainPtr<CGMutablePathRef> m_path;
        mutable bool m_shouldCopyPathBeforeMutation { false };
#endif

Call sites that wanted to prevent wastefully creating an empty CG path would have to check isEmpty explicitly.

Separately, as a clean up step at some point, I it would be slightly better separation of concerns if the platform library provided a function that returned a UIBezierPath. Does not seem like DragDropInteractionState should have to write that code on the spot. This would be analogous to the nsColor function for Color, the NSURL and NSString functions in WTF::URL and WTF::String. Not sure what the best design pattern is, though. Should it be a Path member function? A free function? Which header would we put it in?

There seem to be various callers of platformPath that assume it will never be null without calling ensurePlatformPath, such as PlatformCALayerCocoa::setShapePath.

[Wenson Hsieh]

Created attachment 392147 [details]
v2

[Darin Adler]

Comment on attachment 392147 [details]
v2

New patch looks good. Aside from possibly doing a wasteful copy in the DragDropInteractionState::previewForDragItem code path, which seems like a minor issue we should not worry about

[Darin Adler]

Comment on attachment 392147 [details]
v2

New patch looks good. Aside from possibly doing a wasteful copy in the DragDropInteractionState::previewForDragItem code path, which seems like a minor issue we should not worry about

[Simon Fraser (smfr)]

Comment on attachment 392147 [details]
v2

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

> Source/WebCore/platform/graphics/cg/PathCG.cpp:98
> +        CFRelease(m_path);

Weird that we don't use RetainPtr<> here (via some more platform #ifdeffing).

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

Ew.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:245
> +    CFRelease(m_path);

CFRelease is not null-safe but I guess isEmpty() checked that.

[Wenson Hsieh]

Comment on attachment 392147 [details]
v2

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

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:98
>> +        CFRelease(m_path);
> 
> Weird that we don't use RetainPtr<> here (via some more platform #ifdeffing).

Yes, it’s certainly a strange omission.

I will clean this up in the future (and get rid of all the manual CFRelease/CFRetaining).

[WebKit Commit Bot]

Comment on attachment 392147 [details]
v2

Clearing flags on attachment: 392147

Committed r257732: <https://trac.webkit.org/changeset/257732>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 392147 [details]
v2

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

> Source/WebCore/platform/graphics/Path.h:219
> +    void initializeOrCopyPlatformPathIfNeeded();

I think we should merge this function with ensurePlatformPath.

[Wenson Hsieh]

Reopening to attach new patch.

[Wenson Hsieh]

Created attachment 392280 [details]
Address comment

[Darin Adler]

Comment on attachment 392280 [details]
Address comment

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

I would have used a new bug report for this refinement. Thanks for doing it!

> Source/WebCore/platform/graphics/cg/PathCG.cpp:109
> +            auto pathToCopy = m_path;
> +            m_path = CGPathCreateMutableCopy(pathToCopy);
> +            CFRelease(pathToCopy);

This is going to get simpler when we change m_path to a RetainPtr. It will just be:

    m_path = CGPathCreateMutableCopy(m_path.get());

However in the mean time, I can’t resist pointing out this alternate, insane, one-line way to write it, even though I don’t think you will choose it:

    CFRelease(std::exchange(m_path, CGPathCreateMutableCopy(m_path)));

[Wenson Hsieh]

Comment on attachment 392280 [details]
Address comment

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

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:109
>> +            CFRelease(pathToCopy);
> 
> This is going to get simpler when we change m_path to a RetainPtr. It will just be:
> 
>     m_path = CGPathCreateMutableCopy(m_path.get());
> 
> However in the mean time, I can’t resist pointing out this alternate, insane, one-line way to write it, even though I don’t think you will choose it:
> 
>     CFRelease(std::exchange(m_path, CGPathCreateMutableCopy(m_path)));

That's...certainly more compact. I suppose there's no harm in going for the one-liner for now; as you mentioned, it'll all be much simpler once I make m_path a RetainPtr, anyways.

[Wenson Hsieh]

Created attachment 392284 [details]
Address comment

[WebKit Commit Bot]

Comment on attachment 392284 [details]
Address comment

Clearing flags on attachment: 392284

Committed r257789: <https://trac.webkit.org/changeset/257789>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.