Closed Bug 1118024 Opened 5 years ago Closed 5 years ago

add explicit PL_DHashTable{Add,Lookup,Remove} functions

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: froydnj, Assigned: michael, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(2 files)

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.
Whiteboard: [lang=c++]
Attached patch Patch Part 1Splinter Review
Attached is a patch which introduces the new PL_DHashTable{Add,Lookup,Remove} functions and uses them within xpcom.
Attachment #8544503 - Flags: review?(nfroyd)
Attached patch Patch Part 2Splinter Review
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)
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+
Attachment #8544505 - Flags: review?(nfroyd) → review+
(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!
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
https://hg.mozilla.org/mozilla-central/rev/b61a510edfb9
https://hg.mozilla.org/mozilla-central/rev/fd2f17917aae
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
I'm late to the party on this one, but still: nice!

And I see you filed bug 1120262 as a follow-up. Excellent.
Blocks: 1121202
You need to log in before you can comment on or make changes to this bug.