Closed Bug 1224038 Opened 9 years ago Closed 9 years ago

Use stable hashing for ObjectGroupCompartment::NewTable

Categories

(Core :: JavaScript: GC, defect)

Product:
Core
Component:
JavaScript: GC
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox45 --- fixed
firefox48 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(2 files, 1 obsolete file)

add_infallibe_uid_getters_to_simplify_usage-v0.diff
4.49 KB, patch
sfink
: review+
Details | Diff | Splinter Review
stable_ObjectGroupCompartment_NewEntry-v1.diff
15.47 KB, patch
sfink
: review+
Details | Diff | Splinter Review
This one looks like it's going to be a bit complicated.
It passes tests locally. Try run link above.
Attachment #8688044 - Flags: review?(jcoppeard)
Comment on attachment 8688044 [details] [diff] [review] stable_ObjectGroupCompartmentNewTable-v0.diff Review of attachment 8688044 [details] [diff] [review]: ----------------------------------------------------------------- Great, much nicer.
Attachment #8688044 - Flags: review?(jcoppeard) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3d2ddb4979c9909e012adafd607fc4463e32e6b Bug 1224038 - Use stable hashing in ObjectGroupCompartment::NewTable; r=jonco
A regression/improvement was reported on AWFY: - slave: Mac OS X 10.10 32-bit (Mac Pro, shell) - mode: Ion Regression(s)/Improvement(s): - octane: CodeLoad: -1.38% (regression) - octane: Box2D: -7.58% (regression) - octane: Typescript: -7.21% (regression) Recorded range: - http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a621c6d49da1&tochange=f3d2ddb4979c More details: http://arewefastyet.com/regressions/#/regression/1799692
https://hg.mozilla.org/mozilla-central/rev/f3d2ddb4979c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Confirmed fixed with backout
(In reply to Hannes Verschore [:h4writer] from comment #5) > Recorded range: > - > http://hg.mozilla.org/integration/mozilla-inbound/ > pushloghtml?fromchange=a621c6d49da1&tochange=f3d2ddb4979c Since awfy now can work on revision granularity instead of push granularity the regression range reported was empty. Test to see if this new link works better: - http://hg.mozilla.org/integration/mozilla-inbound/log?rev=a621c6d49da1%3A%3Af3d2ddb4979c%20and%20!a621c6d49da1
Okay, so I've got the perf up to snuff, at least locally. The first part is just some new API to make the implementation of the second part easier.
Attachment #8688044 - Attachment is obsolete: true
Attachment #8710676 - Flags: review?(sphink)
As we discussed on IRC, this pre-caches the lookup UIDs so that we only have to hit the second level table once per lookup. There's still second-level hashing for the key when matching, but balanced against the faster barriers, this seems to break even.
Attachment #8710678 - Flags: review?(sphink)
Attachment #8710676 - Flags: review?(sphink) → review+
Comment on attachment 8710678 [details] [diff] [review] stable_ObjectGroupCompartment_NewEntry-v1.diff Review of attachment 8710678 [details] [diff] [review]: ----------------------------------------------------------------- Very nice!
Attachment #8710678 - Flags: review?(sphink) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d90b6b2c0ef5ed63706675ab7926d623008ba95 Backout f8c3e1e0e597515ee78c04d561a88669054710d9 (Bug 1224038) for bustage on a CLOSED TREE. https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e2fa50448fc750e8696bbea560e7abbe8d4fb9 Backout 27a5ca4a5e74883481ff4fb1bd330d6af41312c9 (Bug 1224038) for bustage on a CLOSED TREE.
https://treeherder.mozilla.org/logviewer.html#?job_id=20530143&repo=mozilla-inbound RecursionGuard failing. I guess NewTable is used from the parsing thread.
Still crashing on the UID table. I'm not seeing that crash or any TSAN errors in the same place locally. But then I have a 60MiB file filled with TSAN errors from the first 50000 mochitests, so it's quite possible I just am not seeing it.
I don't think it should be required, but let's try it with SequentiallyConsistent. All the articles I could find online for counters use it, so maybe there's a potential ordering I'm not considering?
It's still busted, but I strongly suspect this may be related to bug 1232229, as the assertion here could only possibly happen if we touched a zone from multiple threads at once.
No sign of that race anymore, so I think my theory in comment 21 is correct.
Failed on various SM(arm) and SM(e) on m-i that did not fail in try. Must be something that landed in the last day or so.
This was a bad interaction with bug 1252329. I missed the fact that the Entry fixupAfterMovingGC pulls out is a copy not a ref, so I was not updating |associated|. Oddly, the test only fails locally in clang and not in gcc, explaining why I missed it locally. In any case, everything else seemed to be green, so I'm going to try relanding.
https://hg.mozilla.org/mozilla-central/rev/84e0586b8c81 https://hg.mozilla.org/mozilla-central/rev/b20234ac6cf4
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: