Bug 115733 – Avoid unnecessary arguments copying inside GenericHashTraits methods

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Mikhail Pozdnyakov]

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)

[Mikhail Pozdnyakov]

Created attachment 200922 [details]
patch

[Benjamin Poulain]

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.

[Mikhail Pozdnyakov]

(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

[Mikhail Pozdnyakov]

Created attachment 201050 [details]
patch v2

Improved change log.

[Darin Adler]

Comment on attachment 201050 [details]
patch v2

Change seems OK.

[Darin Adler]

I believe newer compilers would optimize such temporaries out. I am really surprised the EFL compiler does not do so!

[Mikhail Pozdnyakov]

(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

[WebKit Commit Bot]

Comment on attachment 201050 [details]
patch v2

Clearing flags on attachment: 201050

Committed r149738: <http://trac.webkit.org/changeset/149738>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

Thank you for updating the ChangeLog.