Closed Bug 1200732 Opened 5 years ago Closed 4 years ago

Use stable hashing for AutoCycleDetector

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox43 --- affected
firefox45 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file)

Use stable hashing for AutoCycleDetector.
Attachment #8655553 - Flags: review?(jcoppeard)
Attachment #8655553 - Flags: review?(jcoppeard) → review+
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.
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.
The cgc failure in the last try run is from bug 1223490 and is fixed with the last patch there.
https://hg.mozilla.org/mozilla-central/rev/f35d1107fe2e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Is it possible that this regressed a few Dromaeo benchmarks on AWFY? (3d-cube, access-binary-trees, deltablue and raytrace)?
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
      ^
You need to log in before you can comment on or make changes to this bug.