Closed Bug 1224050 Opened 4 years ago Closed 4 years ago
Use stable hashing for Initial
This is one of the ugly ones that doesn't show up with a compilation failure because it open codes the hashing of the TaggedProto. I have a patch lying about that didn't work before that I need to rebase. It may "just work" now.
Yup, appears to work now.
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Attachment #8686845 - Flags: review?(jcoppeard)
Comment on attachment 8686845 [details] [diff] [review] stable_InitialShapeTable-v1.diff Review of attachment 8686845 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Shape.cpp @@ -1388,5 @@ > -/* > - * This class is used to add a post barrier on the initialShapes set, as the key > - * is calculated based on objects which may be moved by generational GC. > - */ > -class InitialShapeSetRef : public BufferableRef It's so good to get rid of all of this! @@ +1459,5 @@ > return nullptr; > > Lookup lookup(clasp, protoRoot, nfixed, objectFlags); > + if (!p.add(cx, table, lookup, InitialShapeEntry(ReadBarriered<Shape*>(shape), > + ReadBarriered<TaggedProto>(protoRoot)))) I think we make the ReadBarriered constructor implicit, so can we just pass shape and protoRoot here? ::: js/src/vm/TaggedProto.cpp @@ +48,5 @@ > + HashNumber hn; > + if (!obj->zone()->getHashCode(obj, &hn)) > + oomUnsafe.crash("failed to get hash code for TaggedProto"); > + return hn; > +} It seems like hashCode() duplicates much of the logic in uniqueId(). Can we just call the latter and munge the result?
Attachment #8686845 - Flags: review?(jcoppeard) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/40a37cb11607a03baa857eb41095956923676e5c Bug 1224050 - Use stable hashing for the IntialShapesTable; 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: EarleyBoyer: -4.8% (regression) Recorded range: - http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=43ca206bb812&tochange=40a37cb11607 More details: http://arewefastyet.com/regressions/#/regression/1799694
https://hg.mozilla.org/integration/mozilla-inbound/rev/9aa45a7563473b25a5e9041981b21d61545d707b Backout 40a37cb11607 (Bug 1224050) for regressing the rest of the things.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Confirmed fixed with backout
this caused a lot of talos regressions, mostly in startup: http://alertmanager.allizom.org:8080/alerts.html?rev=02919b7d57d7f8206328f53ac0f7b2b9fe334c9b&showAll=1&testIndex=0&platIndex=0 keep that in mind when relanding!
https://hg.mozilla.org/integration/mozilla-inbound/rev/676b6db552c894a6b2598285f5a1dd3ba5a0f201 Bug 1224050 - Use stable hashing for the IntialShapesTable; r=jonco
I was not able to reproduce any of the regressions locally, but I applied the same optimizations that worked for the ObjectGroupCompartment::NewTable, so I am hopeful that this will take care of the problem here too.
You need to log in before you can comment on or make changes to this bug.