1442480 - Switch nsNodeInfoHash to a nsDataHashTable

Status: RESOLVED FIXED

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

Description

[Eric Rahm [:erahm]]

We should really avoid using PL_Hash if possible. Converting `nsNodeInfoHash` is pretty straightforward.

Comment 1

[Eric Rahm [:erahm]]

try with talos: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6faed20718c39a96cd2244195579efdd7cc8deb4

Comment 2

[Eric Rahm [:erahm]]

Attachment: Switch nsNodeInfoHash to a nsDataHashTable

This switches `nsNodeInfoHash` to a nsDataHashTable. The hash function and
equality operator are moved to NodeInfoInner so that they can be easily reused.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 4

[Eric Rahm [:erahm]]

(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.

Comment 5

[Eric Rahm [:erahm]]

Sorry, references:

[1] https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/xpcom/ds/PLDHashTable.h#44
[2] https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/xpcom/ds/nsAtom.h#76
[3] https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/dom/base/nsNodeInfoManager.cpp#290

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 7

[Eric Rahm [:erahm]]

(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.

Comment 8

[Eric Rahm [:erahm]]

Attachment: Switch nsNodeInfoHash to a nsDataHashTable

I added the cached hash back as a Mabye<uint32_t> and removed a perf TODO comment as we are caching again.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8957016 [details] [diff] [review]
Switch nsNodeInfoHash to a nsDataHashTable

>+    uint32_t Hash() const {
Nit, { goes to its own line

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea89e8713941
Switch nsNodeInfoHash to a nsDataHashTable. r=smaug

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/ea89e8713941