1223863 - Use stable hashing for WeakGlobalObjectSet

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: stable_WeakGlobalObjectSet-v0.diff

Although there are no GGC implications for a Tenured-only Set, we do still need to handle compacting GC. This allows us to remove the manual rekey in Debugger::markAll.

Comment 1

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef0d45dc6245

Comment 2

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e67164e96a65

Comment 3

[Terrence Cole [:terrence]]

This turns up a *really* strange hazard. We have code of the form:

if (dbg->enabled && dbg->debuggees.lookup(rootedGlobal.get())) {
   ... stuff that GC's ...
}

We get a hazard on the last line because ~ReadBarriered<GlobalObject*>() for some temporary created in the first line. This is extremely baffling. Note that the patch *does not* change the key type, only the hasher type. The only thing I can think is that the type of Lookup changed, however that should not matter here: the temporaries from argument evaluation should be dead by the sequence point between the call result and the evaluation of the if-statement condition.

Theory #1: The argument temporary is living too long. Moreover, the temporary created by rootedGlobal.get() is a raw GlobalObject*, which would be a hazard itself if the theory above were correct. It has not been a hazard before, so this seems unlikely, however theory #2 is even crazier.

Theory #2: after the evaluation, HashTable::Lookup returns a HashTable::Ptr, which might feasibly live for the entire loop; it's not named so I wouldn't expect it to, but whatever, at least it's within spec. Massive *however*: the type of HashTable::Ptr should not have changed with this patch, so it should have been a hazard before too.

Ugh!

The analysis isn't running for me locally anymore, so I've sent off for a shell hazard analysis with the .lookup changed to .has. HashTable::has returns bool after evaluating lookup internally, so this should disprove at least one of the theories above.

Comment 4

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=540ab98f7258

Seems the prior try stanza didn't trigger the builds I wanted. Let's try tacking an 'o' on.

Comment 5

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68ce818e9a1c

Comment 6

[Terrence Cole [:terrence]]

Okay, theory #2 is disproved. Let's try pulling the temporary, call, and result all the way above the if-statement.

Comment 7

[Terrence Cole [:terrence]]

Huh, that seems to have solved it. Will upload the new patch.

Comment 8

[Terrence Cole [:terrence]]

Attachment: stable_WeakGlobalObjectSet-v1.diff

Comment 9

[Jon Coppeard (:jonco)]

Comment on attachment 8686326 [details] [diff] [review]
stable_WeakGlobalObjectSet-v1.diff

Review of attachment 8686326 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +3129,5 @@
>      ExecutionObservableCompartments obs(cx);
>      if (!obs.init())
>          return false;
>  
> +    if (dbg->debuggees.has(global.get())) {

Looking at these get()s again, can we define specialisations of MovableCellHasher like we do for DefaultHasher so that the Lookup type is the raw pointer?

Comment 10

[Terrence Cole [:terrence]]

Brilliant! Works like a charm!

Comment 11

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4234cbfaf2f990e104d9abf3cf24c08780f95cfa
Bug 1223863 - Use stable hashing for WeakGlobalObjectSet; r=jonco

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4234cbfaf2f9