908709 - GenerationalGC: Can't reuse hash when inserting into newTypeObjects set

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: new-type-object-keys

In ExclusiveContext::getNewType() we currently use lookupForAdd() and relookOrAdd() when inserting into this set, which allows us to calculate the hash once for lookup, and reuse it for add.

Unfortunately, the set is keyed in part on the prototype object which may be moved when we allocate the new type object in between these operations.  So the original hash may not still be valid.

Here is a patch to remove this optimisation.

In addition, I think there is a further problem lurking here, in that the prototype can subsequently be moved, and this won't currently rekey the hash.  Solving that will probably require adding a post barrier callback, but I haven't worked out the details yet.

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 794722 [details] [diff] [review]
new-type-object-keys

Review of attachment 794722 [details] [diff] [review]:
-----------------------------------------------------------------

Lets do what till suggested here and check the gcNumber before and after to see if we can safely re-lookupOrAdd.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: new-type-object-keys

Updated patch with till's suggested optimization.

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 796008 [details] [diff] [review]
new-type-object-keys

Review of attachment 796008 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsinfer.cpp
@@ +6085,5 @@
> +     */
> +    bool gcHappened = gcNumber() != originalGcNumber;
> +    bool added =
> +        gcHappened ? newTypeObjects.putNew(TypeObjectSet::Lookup(clasp, proto), type.get())
> +                   : newTypeObjects.relookupOrAdd(p, TypeObjectSet::Lookup(clasp, proto), type.get());

I've no idea if that'd make a difference (because I still don't know how hot this code is), but reformulating this in terms of if/else would enable wrapping the gcHappened in JS_UNLIKELY.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 796008 [details] [diff] [review]
new-type-object-keys

Review of attachment 796008 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: js/src/jsinfer.cpp
@@ +6085,5 @@
> +     */
> +    bool gcHappened = gcNumber() != originalGcNumber;
> +    bool added =
> +        gcHappened ? newTypeObjects.putNew(TypeObjectSet::Lookup(clasp, proto), type.get())
> +                   : newTypeObjects.relookupOrAdd(p, TypeObjectSet::Lookup(clasp, proto), type.get());

JS_UNLIKELY is unlikely to have a significant performance effect, even if this is super hot. I think the ternary control flow reads slightly better than an if would, considering the need to set a bool and check it afterwards.

Comment 5

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7dc1afc3c4c

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c7dc1afc3c4c