| Summary: | Avoid unnecessary arguments copying inside GenericHashTraits methods | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mikhail Pozdnyakov <mikhail.pozdnyakov> | ||||||
| Component: | Web Template Framework | Assignee: | Mikhail Pozdnyakov <mikhail.pozdnyakov> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | andersca, benjamin, cmarcelo, commit-queue, darin | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
Created attachment 200922 [details]
patch
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.
(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 Created attachment 201050 [details]
patch v2
Improved change log.
Comment on attachment 201050 [details]
patch v2
Change seems OK.
I believe newer compilers would optimize such temporaries out. I am really surprised the EFL compiler does not do so! (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 on attachment 201050 [details] patch v2 Clearing flags on attachment: 201050 Committed r149738: <http://trac.webkit.org/changeset/149738> All reviewed patches have been landed. Closing bug. Thank you for updating the ChangeLog. |
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)