Bug 87938 - [GTK] Avoid unnecessary heap allocations during drag and drop operations
Summary: [GTK] Avoid unnecessary heap allocations during drag and drop operations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-31 01:10 PDT by Carlos Garcia Campos
Modified: 2012-12-04 02:34 PST (History)
3 users (show)

See Also:


Attachments
Patch (14.29 KB, patch)
2012-05-31 01:17 PDT, Carlos Garcia Campos
mrobinson: review-
Details | Formatted Diff | Diff
Updated patch to apply on current git master (14.24 KB, patch)
2012-12-03 07:58 PST, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2012-05-31 01:10:33 PDT
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
Comment 1 Carlos Garcia Campos 2012-05-31 01:17:11 PDT
Created attachment 145013 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-31 01:19:40 PDT
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
Comment 3 Martin Robinson 2012-05-31 06:07:35 PDT
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.
Comment 4 Martin Robinson 2012-05-31 06:09:07 PDT
Comment on attachment 145013 [details]
Patch

An alternative approach would be to cache the DragData in DragData in DragAndDropHelper.
Comment 5 Carlos Garcia Campos 2012-05-31 08:16:20 PDT
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.
Comment 6 Carlos Garcia Campos 2012-05-31 08:25:16 PDT
(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.
Comment 7 Carlos Garcia Campos 2012-06-01 08:54:19 PDT
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.
Comment 8 Martin Robinson 2012-06-01 08:59:59 PDT
(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.
Comment 9 Carlos Garcia Campos 2012-06-01 09:03:17 PDT
(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.
Comment 10 Carlos Garcia Campos 2012-12-03 07:58:28 PST
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.
Comment 11 Carlos Garcia Campos 2012-12-04 02:34:02 PST
Committed r136494: <http://trac.webkit.org/changeset/136494>