Closed Bug 1224038 Opened 6 years ago Closed 6 years ago

Use stable hashing for ObjectGroupCompartment::NewTable


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox45 --- fixed
firefox48 --- fixed


(Reporter: terrence, Assigned: terrence)




(2 files, 1 obsolete file)

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]

Review of attachment 8688044 [details] [diff] [review]:

Great, much nicer.
Attachment #8688044 - Flags: review?(jcoppeard) → review+
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

- octane: CodeLoad: -1.38% (regression)
- octane: Box2D: -7.58% (regression)
- octane: Typescript: -7.21% (regression)

Recorded range:

More details:
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Resolution: FIXED → ---
Confirmed fixed with backout
(In reply to Hannes Verschore [:h4writer] from comment #5)
> Recorded range:
> -
> 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:
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]

Review of attachment 8710678 [details] [diff] [review]:

Very nice!
Attachment #8710678 - Flags: review?(sphink) → review+
Backout f8c3e1e0e597515ee78c04d561a88669054710d9 (Bug 1224038) for bustage on a CLOSED TREE.
Backout 27a5ca4a5e74883481ff4fb1bd330d6af41312c9 (Bug 1224038) for bustage on a CLOSED TREE.

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.
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.