add explicit PL_DHashTable{Add,Lookup,Remove} functions

RESOLVED FIXED in mozilla37

Status

()

Core
XPCOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: froydnj, Assigned: Michael Pruett, Mentored)

Tracking

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++])

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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

4 years ago
Whiteboard: [lang=c++]
(Assignee)

Comment 1

4 years ago
Created attachment 8544503 [details] [diff] [review]
Patch Part 1

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

4 years ago
Created attachment 8544505 [details] [diff] [review]
Patch Part 2

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

4 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

4 years ago
Attachment #8544505 - Flags: review?(nfroyd) → review+
(Reporter)

Comment 4

4 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

4 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
https://hg.mozilla.org/mozilla-central/rev/b61a510edfb9
https://hg.mozilla.org/mozilla-central/rev/fd2f17917aae
Status: NEW → RESOLVED
Last Resolved: 4 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.
(Reporter)

Updated

4 years ago
Blocks: 1121202
Blocks: 1128407
You need to log in before you can comment on or make changes to this bug.