Bug 103650 – [CoordinatedGraphics] Use OwnPtr for LayerMap's layers in LayerTreeRenderer

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

We currently store raw pointers to WebCore::GraphicsLayer in the LayerMap meaning that we need to delete them manually.
We could use smart pointers and store them as OwnPtr in the LayerMap. The LayerMap would own the layers and we would not need to handle memory manually.

[Chris Dumez]

Created attachment 176759 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 176759 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h:183
> +    typedef HashMap< WebLayerID, OwnPtr<WebCore::GraphicsLayer> > LayerMap;

I dont think the first space is needed

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h:185
> +    typedef HashMap<WebLayerID, WebCore::GraphicsLayer*> RawLayerMap;

LayerPtrMap ?

[Kenneth Rohde Christiansen]

Comment on attachment 176759 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:-375
>      m_fixedLayers.remove(layerID);
>  #if USE(GRAPHICS_SURFACE)
>      m_surfaceBackingStores.remove(layerID);
>  #endif
> -    delete layer;

so how are they stored here

[Chris Dumez]

Comment on attachment 176759 [details]
Patch

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

>> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:-375
>> -    delete layer;
> 
> so how are they stored here

Sorry, I don't get the question. Is the comment on the right line?
I'm merely removing the explicit deletion here since it will get deleted when the OwnPtr goes out of scope.

>> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h:183
>> +    typedef HashMap< WebLayerID, OwnPtr<WebCore::GraphicsLayer> > LayerMap;
> 
> I dont think the first space is needed

It is not but I like symmetry :) Anyway, if the other way is more common I will change.

>> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h:185
>> +    typedef HashMap<WebLayerID, WebCore::GraphicsLayer*> RawLayerMap;
> 
> LayerPtrMap ?

Is it really clearer? They are both pointers (Ptr), the different is that one is raw and the other isn't. LayerRawPtrMap ?

[Kenneth Rohde Christiansen]

(In reply to comment #4)
> (From update of attachment 176759 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176759&action=review
> 
> >> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:-375
> >> -    delete layer;
> > 
> > so how are they stored here
> 
> Sorry, I don't get the question. Is the comment on the right line?
> I'm merely removing the explicit deletion here since it will get deleted when the OwnPtr goes out of scope.

I was refering to m_surfaceBackingStores.remove(layerID); but that is out of scope for this patch


> >> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h:183
> >> +    typedef HashMap< WebLayerID, OwnPtr<WebCore::GraphicsLayer> > LayerMap;
> > 
> > I dont think the first space is needed
> 
> It is not but I like symmetry :) Anyway, if the other way is more common I will change.

Dont do this, it is uncommon and as you can see not done in RawLayerMap

> 
> >> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h:185
> >> +    typedef HashMap<WebLayerID, WebCore::GraphicsLayer*> RawLayerMap;
> > 
> > LayerPtrMap ?
> 
> Is it really clearer? They are both pointers (Ptr), the different is that one is raw and the other isn't. LayerRawPtrMap ?

yes I like LayerRawPtrMap more

[Chris Dumez]

Created attachment 176771 [details]
Patch

Take Kenneth's feedback into consideration.

[Noam Rosenthal]

Comment on attachment 176771 [details]
Patch

This is fine with me. Leaving cq still at ? in case Kenneth has more comments :)

[Mikhail Pozdnyakov]

Comment on attachment 176771 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:366
> +    OwnPtr<GraphicsLayer> layer = m_layers.take(layerID);

Is it OwnPtr(const OwnPtr<ValueType>&); ?
        // This copy constructor is used implicitly by gcc when it generates
        // transients for assigning a PassOwnPtr<T> object to a stack-allocated
        // OwnPtr<T> object. It should never be called explicitly and gcc
        // should optimize away the constructor when generating code.

[Chris Dumez]

Comment on attachment 176771 [details]
Patch

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

>> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:366
>> +    OwnPtr<GraphicsLayer> layer = m_layers.take(layerID);
> 
> Is it OwnPtr(const OwnPtr<ValueType>&); ?
>         // This copy constructor is used implicitly by gcc when it generates
>         // transients for assigning a PassOwnPtr<T> object to a stack-allocated
>         // OwnPtr<T> object. It should never be called explicitly and gcc
>         // should optimize away the constructor when generating code.

take() returns a PassOwnPtr here so it is fine.

[WebKit Review Bot]

Comment on attachment 176771 [details]
Patch

Clearing flags on attachment: 176771

Committed r136181: <http://trac.webkit.org/changeset/136181>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dongseong Hwang]

I'm afraid whether it is safe for HashMap to contain OwnPtr.

AFAIK, Vector<OwnPtr<T> > is unsafe because relocating can call destructor and constructor.

I don't know detail of HashMap implementation, so I can not say it is unsafe surely. However, I'm afraid.

In common, we don't store autoptr in stl::HashMap.

[Chris Dumez]

(In reply to comment #12)
> I'm afraid whether it is safe for HashMap to contain OwnPtr.
> 
> AFAIK, Vector<OwnPtr<T> > is unsafe because relocating can call destructor and constructor.
> 
> I don't know detail of HashMap implementation, so I can not say it is unsafe surely. However, I'm afraid.
> 
> In common, we don't store autoptr in stl::HashMap.

The WTF::HashMap has a specific behaviour for OwnPtr defined in HashTraits.h. I don't think this is unsafe and it is also used in other places in WebKit.

[Dongseong Hwang]

(In reply to comment #13)
> (In reply to comment #12)
> > I'm afraid whether it is safe for HashMap to contain OwnPtr.
> > 
> > AFAIK, Vector<OwnPtr<T> > is unsafe because relocating can call destructor and constructor.
> > 
> > I don't know detail of HashMap implementation, so I can not say it is unsafe surely. However, I'm afraid.
> > 
> > In common, we don't store autoptr in stl::HashMap.
> 
> The WTF::HashMap has a specific behaviour for OwnPtr defined in HashTraits.h. I don't think this is unsafe and it is also used in other places in WebKit.

Thanks for explanation!