Closed
Bug 1118024
Opened 9 years ago
Closed 9 years ago
add explicit PL_DHashTable{Add,Lookup,Remove} functions
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: froydnj, Assigned: michael, Mentored)
References
Details
(Whiteboard: [lang=c++])
Attachments
(2 files)
8.10 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
71.43 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
We have this weird setup in pldhash.cpp where all the normal hash table operations go through PL_DHashTableOperate: http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/pldhash.cpp#665 What this means in practice is that all hash table operations have to pay the runtime cost for the switch dispatch in that function, even though the desired operation is nearly always known at compile time. That's not good. Fortunately, compilers nowadays are smart enough to inline functions with known-constant parameters, which gets rid of the overhead. However, this likely only happens in LTO builds; we should make it happen all the time by rearranging the code. This has slight readability improvements in addition to the small performance benefit. Fixing this comes in two parts: 1. Define PL_DHashTable{Add,Lookup,Remove} functions that, just as PL_DHashTableOperate does, forward to PLDHashTable::Operate with the appropriate op. 2. Fixing all the callsites that call PL_DHashTableOperate with a compile-time constant op to call our new functions instead.
Reporter | ||
Updated•9 years ago
|
Whiteboard: [lang=c++]
Assignee | ||
Comment 1•9 years ago
|
||
Attached is a patch which introduces the new PL_DHashTable{Add,Lookup,Remove} functions and uses them within xpcom.
Attachment #8544503 -
Flags: review?(nfroyd)
Assignee | ||
Comment 2•9 years ago
|
||
And here's a patch which updates callers of PL_DHashTableOperate() to use the new PL_DHashTable{Add,Lookup,Remove} functions. Here are the results from the try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b0ed89a986c
Attachment #8544505 -
Flags: review?(nfroyd)
Reporter | ||
Comment 3•9 years ago
|
||
Comment on attachment 8544503 [details] [diff] [review] Patch Part 1 Review of attachment 8544503 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for doing this!
Attachment #8544503 -
Flags: review?(nfroyd) → review+
Reporter | ||
Updated•9 years ago
|
Attachment #8544505 -
Flags: review?(nfroyd) → review+
Reporter | ||
Comment 4•9 years ago
|
||
(In reply to Michael Pruett from comment #2) > Here are the results from the try server: > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b0ed89a986c Everything looks good on this push except for the dt1 oranges, which don't look entirely explainable in terms of known intermittent failures. I've retriggered the test job several times, and ideally those results will come back green. Assuming those come back green, we can mark the bug as checkin-needed. Thanks for the patches!
Reporter | ||
Comment 5•9 years ago
|
||
Retriggers look green, and consulting with a sheriff suggests that the oranges are nothing to worry about. Marking checkin-needed!
Assignee: nobody → michael
Keywords: checkin-needed
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b61a510edfb9 https://hg.mozilla.org/integration/mozilla-inbound/rev/fd2f17917aae
Keywords: checkin-needed
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b61a510edfb9 https://hg.mozilla.org/mozilla-central/rev/fd2f17917aae
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 8•9 years ago
|
||
I'm late to the party on this one, but still: nice! And I see you filed bug 1120262 as a follow-up. Excellent.
Updated•9 years ago
|
Blocks: modernize-pldhash
You need to log in
before you can comment on or make changes to this bug.
Description
•