1200732 - Use stable hashing for AutoCycleDetector

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Use stable hashing for AutoCycleDetector.

Comment 1

[Terrence Cole [:terrence]]

Attachment: stable_AutoCycleDetectorSet-v0.diff

Comment 2

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d92d88957742

Comment 3

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96691a3775ed

Comment 4

[Terrence Cole [:terrence]]

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

Comment 5

[Terrence Cole [:terrence]]

Ah, yes, that's what was going wrong: self hosted cloning.

When in a worker, if we, for example, try to access the ToObject intrinsic, this clones from the self-hosting zone... of the main-threads shared, self-hosting zone. This is not a cross-zone access, but a cross-runtime and cross-thread access. I'm not sure how this code prevents a compacting GC from moving all the cells it's copying from? But whatever. In this specific case, the MovableCellHasher is trying to add unique ids to cells in the main thread's self-hosting zone from a different thread and failing (correctly) in CurrentThreadCanAccessZone.

Comment 6

[Terrence Cole [:terrence]]

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

Conceptually, we are moving the hash id from being owned by the hashtable (the pointers) to being owned by the object (the uid). This effectively moves the ownership across threads in this case. Whoops!

Given that the is-there-a-cycle test is not going to be different across threads (or even after the first access), and given that a failure of the cycle detector to detect the cycle will just be an immediate, reproducible OOM crash, I think we'd be totally fine just not doing the cycle detection when cloning into a different thread.

Comment 7

[Terrence Cole [:terrence]]

Derp: https://treeherder.mozilla.org/#/jobs?repo=try&revision=80d7aac98d2b

Comment 8

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f35d1107fe2eabc3128c9430724fa730c3336fd5
Bug 1200732 - Use stable hashing for AutoCycleDetectorSet; r=jonco

Comment 9

[Terrence Cole [:terrence]]

The cgc failure in the last try run is from bug 1223490 and is fixed with the last patch there.

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Guilherme Lima]

Is it possible that this regressed a few Dromaeo benchmarks on AWFY? (3d-cube, access-binary-trees, deltablue and raytrace)?

Comment 12

[Jakob Stoklund Olesen [:jolesen]]

This patch broke the unified build for the ARM simulator. Not sure why it doesn't show up on treeherder:

In file included from /Users/jolesen/gecko-dev/obj-a32simdev/js/src/Unified_cpp_js_src32.cpp:2:
/Users/jolesen/gecko-dev/js/src/vm/SelfHosting.cpp:2028:5: error: no template named 'Maybe'; did you mean 'mozilla::Maybe'?
    Maybe<AutoCycleDetector> detect;
    ^~~~~
    mozilla::Maybe
../../dist/include/mozilla/Maybe.h:83:7: note: 'mozilla::Maybe' declared here
class Maybe
      ^