Closed Bug 1442480 Opened 2 years ago Closed 2 years ago

Switch nsNodeInfoHash to a nsDataHashTable

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

We should really avoid using PL_Hash if possible. Converting `nsNodeInfoHash` is pretty straightforward.
This switches `nsNodeInfoHash` to a nsDataHashTable. The hash function and
equality operator are moved to NodeInfoInner so that they can be easily reused.
Attachment #8955370 - Flags: review?(bugs)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Blocks: 1442752
Comment on attachment 8955370 [details] [diff] [review]
Switch nsNodeInfoHash to a nsDataHashTable

I don't understand why you get rid of mHashInitialized.
Am I missing something, is the hash value cached somewhere else?
Attachment #8955370 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] (unusual timezone [JST] for a week) from comment #3)
> Comment on attachment 8955370 [details] [diff] [review]
> Switch nsNodeInfoHash to a nsDataHashTable
> 
> I don't understand why you get rid of mHashInitialized.
> Am I missing something, is the hash value cached somewhere else?

Yes the hash value is cached in the hash table entry [1], it's cached for lookups if the key is an atom [2], but it won't be cached for lookups by string name [3] (where we calculate the hash for the MRU table and then again for the table lookup). So if it's in MRU or the key is an atom it's a non-issue.

I can add it back, but it seemed overkill. I know Ehsan added as a perf improvement but given the underlying entry type caches now I wasn't sure if we wanted/needed the cache.
This is rather hot code, so I'm just trying to understand in which cases we don't use that cache.
Even before ehsan's changes hash was cached for atoms.

But with this new setup, whenever we add a new thing, we calculate the hash twice.
First to check whether we're in mRecentlyUsedNodeInfos and then when adding to the hashtable.
I don't really see reason to remove the optimization ehsan added.
(In reply to Olli Pettay [:smaug] (unusual timezone [JST] for a week) from comment #6)
> This is rather hot code, so I'm just trying to understand in which cases we
> don't use that cache.
> Even before ehsan's changes hash was cached for atoms.
> 
> But with this new setup, whenever we add a new thing, we calculate the hash
> twice.
> First to check whether we're in mRecentlyUsedNodeInfos and then when adding
> to the hashtable.
> I don't really see reason to remove the optimization ehsan added.

I wanted to avoid storing the hash twice; given it's hot code I'll add it back.
I added the cached hash back as a Mabye<uint32_t> and removed a perf TODO comment as we are caching again.
Attachment #8957016 - Flags: review?(bugs)
Attachment #8955370 - Attachment is obsolete: true
Comment on attachment 8957016 [details] [diff] [review]
Switch nsNodeInfoHash to a nsDataHashTable

>+    uint32_t Hash() const {
Nit, { goes to its own line
Attachment #8957016 - Flags: review?(bugs) → review+
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea89e8713941
Switch nsNodeInfoHash to a nsDataHashTable. r=smaug
https://hg.mozilla.org/mozilla-central/rev/ea89e8713941
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.