1224050 - Use stable hashing for InitialShapeTable

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

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.

Comment 1

[Terrence Cole [:terrence]]

Attachment: stable_InitialShapeTable-v1.diff

Yup, appears to work now.

Comment 2

[Terrence Cole [:terrence]]

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

Comment 3

[Jon Coppeard (:jonco)]

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?

Comment 4

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40a37cb11607a03baa857eb41095956923676e5c
Bug 1224050 - Use stable hashing for the IntialShapesTable; r=jonco

Comment 5

[Hannes Verschore [:h4writer]]

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

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40a37cb11607

Comment 7

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9aa45a7563473b25a5e9041981b21d61545d707b
Backout 40a37cb11607 (Bug 1224050) for regressing the rest of the things.

Comment 8

[Hannes Verschore [:h4writer]]

Confirmed fixed with backout

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

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!

Comment 10

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/676b6db552c894a6b2598285f5a1dd3ba5a0f201
Bug 1224050 - Use stable hashing for the IntialShapesTable; r=jonco

Comment 11

[Terrence Cole [:terrence]]

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.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/676b6db552c8