Bug 87938 – [GTK] Avoid unnecessary heap allocations during drag and drop operations

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

The DragAndDropHelper returns a new allocated DragData object in several methods, while the only thing we need is the platformData (DataObjectGtk). So we could just return a pointer and create the DragData object in the stack

[Carlos Garcia Campos]

Created attachment 145013 [details]
Patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Robinson]

Comment on attachment 145013 [details]
Patch

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

Hrm. I'm not sure if I prefer the new approach. It does avoid an allocation on the heap, but replaces it with an allocation on the stack which is arguably riskier and with the result of creating more code.   Additionally this moves from two places in the code having to create DragData  to six.

I worry also that the use of position as an in and out parameter could be confusing and would prefer to simply have a getter on DragAndDropHelper.

> Source/WebCore/platform/gtk/GtkDragAndDropHelper.cpp:161
> -        return adoptPtr(static_cast<DragData*>(0));
> +        return 0;
>  

I'm not sure what I was thinking when I wrote this code. This should be return nullptr here.

[Martin Robinson]

Comment on attachment 145013 [details]
Patch

An alternative approach would be to cache the DragData in DragData in DragAndDropHelper.

[Carlos Garcia Campos]

Look at how other ports use it, all of them create a DragData in the stack when needed, because I think it's thought as a temporary object.

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 145013 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145013&action=review
> 
> Hrm. I'm not sure if I prefer the new approach. It does avoid an allocation on the heap, but replaces it with an allocation on the stack which is arguably riskier and with the result of creating more code.   Additionally this moves from two places in the code having to create DragData  to six.
> 
> I worry also that the use of position as an in and out parameter could be confusing and would prefer to simply have a getter on DragAndDropHelper.
> 

That's a workaround to use the last motion position instead of the coords passed to drag-data-received, the caller has everything required to build a DragData.

[Carlos Garcia Campos]

DragData has a small size so I don't think there's any risk in allocate it on the stack, and it's what all other ports do after all.

[Martin Robinson]

(In reply to comment #7)
> DragData has a small size so I don't think there's any risk in allocate it on the stack, and it's what all other ports do after all.

It still seems like this is less clean than the old approach.  An allocation on the heap isn't such a terrible thing, but even so it can be eliminated by simply caching the DragData in the DragAndDropHelper.

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #7)
> > DragData has a small size so I don't think there's any risk in allocate it on the stack, and it's what all other ports do after all.
> 
> It still seems like this is less clean than the old approach.  An allocation on the heap isn't such a terrible thing, but even so it can be eliminated by simply caching the DragData in the DragAndDropHelper.

Well, I don't see this is less clean, and DragData looks like a temp object to me, so I prefer not to cache it at all.

[Carlos Garcia Campos]

Created attachment 177257 [details]
Updated patch to apply on current git master

Let's give this another try, I still think the code is simpler, more efficient and consistent with *all* other ports in both WebKit1 and WebKit2.

[Carlos Garcia Campos]

Committed r136494: <http://trac.webkit.org/changeset/136494>