732815 - Don't re-hash the atom in nsNodeInfoManager::GetNodeInfoInnerHashValue

Status: RESOLVED FIXED

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

Description

[Justin Lebar (not reading bugmail)]

nsNodeInfoManager.cpp:

>PLHashNumber
>nsNodeInfoManager::GetNodeInfoInnerHashValue(const void *key)
>{
>  NS_ASSERTION(key, "Null key passed to nsNodeInfo::GetHashValue!");
>
>  const nsINodeInfo::nsNodeInfoInner *node =
>    reinterpret_cast<const nsINodeInfo::nsNodeInfoInner *>(key);
>
>  if (node->mName) {
>    return HashString(nsDependentAtomString(node->mName));
>  }
>  return HashString(*(node->mNameString));
>}

In bug 729940 comment 23, bz says:

> I wish we could use node->mName->hash() [instead of the first call to HashString], but the
> mName and mNameString cases need to produce the same hash when they represent the same 
> string.
>
> We should consider having nsIAtom::hash() return the HashString of the string instead of what 
> it returns right now.  Followup bug on that, maybe?

What nsIAtom::hash() returns right now is described in bug 729940 comment 38:

> For my reference, this is kind of complicated right now because atoms store the 
> pldhashtable's hashcode, which is
>
>  (hash_we_calculate * golden_ratio) >> 1
> 
> plus some complication if hash_we_calculate * golden_ratio == 0 or 1.  The solution would 
> probably be to store hash_we_calculate in the atom and then do the extra computation on it 
> when needed.

Comment 1

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

Attachment: patch

Not super-pretty, but trying to make sure we don't do any extra hash calculations.

Comment 2

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

https://tbpl.mozilla.org/?tree=Try&rev=487c23a766d4

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 815930 [details] [diff] [review]
patch

The atom table changes seem really fragile to me, depending on a bunch of implementation details from pldhash, right?

Comment 4

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

Attachment: v2

This way then

Comment 5

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

Attachment: v3

Comment 6

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

Attachment: v4

Just to fix an assert
https://tbpl.mozilla.org/?tree=Try&rev=fd3bcffc8f3a

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 816083 [details] [diff] [review]
v4

>+    if (aHash) {
>+      mHash = aHash;

Could we assert that the string hashes to the right thing in this case?  Same in the utf-8 case.

r=me with that, and sorry that took so long.  :(

Comment 8

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

Attachment: +asserts

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/043f0cdc3fe7

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/043f0cdc3fe7