Bug 115733 - Avoid unnecessary arguments copying inside GenericHashTraits methods
Summary: Avoid unnecessary arguments copying inside GenericHashTraits methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-07 10:06 PDT by Mikhail Pozdnyakov
Modified: 2013-05-08 13:55 PDT (History)
5 users (show)

See Also:


Attachments
patch (2.02 KB, patch)
2013-05-07 10:15 PDT, Mikhail Pozdnyakov
benjamin: review-
Details | Formatted Diff | Diff
patch v2 (2.68 KB, patch)
2013-05-08 03:20 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Pozdnyakov 2013-05-07 10:06:25 PDT
At the moment we have in GenericHashTraits:

typedef T PassOutType;
static PassOutType passOut(const T& value) { return value; }

and 

typedef T PeekType;
static PeekType peek(const T& value) { return value; }

In case T is a class having non-trivial copy constructor it'll be invoked on exit of both passOut() and peek().
(even with O3 optimization enabled for GCC at least)
Comment 1 Mikhail Pozdnyakov 2013-05-07 10:15:30 PDT
Created attachment 200922 [details]
patch
Comment 2 Benjamin Poulain 2013-05-07 12:51:26 PDT
Comment on attachment 200922 [details]
patch

I fail to see in which case this is relevant. Can you give more explanation in the ChangeLog?

For this kind of patch, you should also include in the ChangeLog how much smaller the binary becomes. For changes in inlines function, the binary size often gives reviewers a good metric on how large the improvement is.
Comment 3 Mikhail Pozdnyakov 2013-05-08 02:32:12 PDT
(In reply to comment #2)
> (From update of attachment 200922 [details])
> I fail to see in which case this is relevant. Can you give more explanation in the ChangeLog?
> 
Sure, I'll provide more explanation.

> For this kind of patch, you should also include in the ChangeLog how much smaller the binary becomes. For changes in inlines function, the binary size often gives reviewers a good metric on how large the improvement is.

Thanks for an interesting metric proposal :) 

That is what 'ls' gave in the size column for libjavascriptcore_efl.so.0.1.0 (EFL release build):

before my change: 6554992
after my change: 6554560
Comment 4 Mikhail Pozdnyakov 2013-05-08 03:20:25 PDT
Created attachment 201050 [details]
patch v2

Improved change log.
Comment 5 Darin Adler 2013-05-08 07:26:04 PDT
Comment on attachment 201050 [details]
patch v2

Change seems OK.
Comment 6 Darin Adler 2013-05-08 07:26:31 PDT
I believe newer compilers would optimize such temporaries out. I am really surprised the EFL compiler does not do so!
Comment 7 Mikhail Pozdnyakov 2013-05-08 07:39:02 PDT
(In reply to comment #6)
> I believe newer compilers would optimize such temporaries out. I am really surprised the EFL compiler does not do so!

That was gcc 4.6.3
Comment 8 WebKit Commit Bot 2013-05-08 08:07:11 PDT
Comment on attachment 201050 [details]
patch v2

Clearing flags on attachment: 201050

Committed r149738: <http://trac.webkit.org/changeset/149738>
Comment 9 WebKit Commit Bot 2013-05-08 08:07:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Benjamin Poulain 2013-05-08 13:55:47 PDT
Thank you for updating the ChangeLog.