Closed Bug 908709 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: jonco, Assigned: jonco)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch new-type-object-keys (obsolete) — Splinter Review
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.
Attachment #794722 - Flags: review?(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.
Attachment #794722 - Flags: review?(terrence)
Updated patch with till's suggested optimization.
Attachment #794722 - Attachment is obsolete: true
Attachment #796008 - Flags: review?(terrence)
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 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.
Attachment #796008 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/c7dc1afc3c4c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.