Closed Bug 1348905 Opened 3 years ago Closed 3 years ago

Fix Octane-TypeScript regression from bug 1328140

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

No description provided.
In IonSetPropertyIC::update there was a second isTemporarilyUnoptimizable bool shadowing the outer one.

Also, we should attach the megamorphic SetProp stub if IsPreliminaryObject(obj) is true. I added that condition but I don't think there's a good reason for it and this case does show up on TypeScript.
Attachment #8849169 - Flags: review?(hv1989)
Attachment #8849169 - Flags: review?(hv1989) → review+
* HasTypePropertyId should just call TrackPropertyTypes instead of duplicating it. I removed TrackPropertyTypes's unused cx argument so we can call it without a cx.

* We should only check nonConstantProperty for singleton objects (this fixes the main regression). See the code/comments in HeapTypeSetKey::couldBeConstant and PropertyHasBeenMarkedNonConstant for example.

* I added MOZ_ALWAYS_INLINE to some functions that weren't being inlined by Clang according to the profiler but really should be.
Attachment #8849171 - Flags: review?(hv1989)
A bonus patch to templatize SetNativeDataProperty to avoid the type barrier if we know we don't need one.

This case shows up on TypeScript and pdf.js and type barriers are relatively slow.
Attachment #8849172 - Flags: review?(hv1989)
Comment on attachment 8849171 [details] [diff] [review]
Part 2 - Some HasTypePropertyId changes

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

::: js/src/vm/TypeInference-inl.h
@@ +415,5 @@
> +        if (!types->hasType(type))
> +            return false;
> +        // Non-constant properties are only relevant for singleton objects.
> +        if (obj->isSingleton() && !types->nonConstantProperty())
> +            return false;

Shouldn't just call PropertyHasBeenMarkedNonConstant() here instead?
return PropertyHasBeenMarkedNonConstant();

PropertyHasBeenMarkedNonConstant has another check that we don't do here. I think it is because we cannot encounter it in this code anymore. As a result that shouldn't be a problem.
Attachment #8849171 - Flags: review?(hv1989) → review+
Comment on attachment 8849172 [details] [diff] [review]
Part 3 - Avoid SetNativeDataProperty type barrier if possible

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

Cool. All these patches were good finds!
Attachment #8849172 - Flags: review?(hv1989) → review+
(In reply to Hannes Verschore [:h4writer] from comment #4)
> Shouldn't just call PropertyHasBeenMarkedNonConstant() here instead?
> return PropertyHasBeenMarkedNonConstant();

I did consider it but this code is really hot and PropertyHasBeenMarkedNonConstant will do another maybeGetProperty call etc that adds extra overhead.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52ed0ff33648
part 1 - Remove unnecessary IsPreliminaryObject check so we attach megamorphic setprop stubs in more cases. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/24cf402f2035
part 2 - Clean up and fix HasTypePropertyId. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c9940c57e33
part 3 - Make SetNativeDataProperty skip the type barrier if the IC knows we don't need one. r=h4writer
AWFY confirms this fixed the Octane-TypeScript regression. It's a bit faster than before actually.
https://hg.mozilla.org/mozilla-central/rev/52ed0ff33648
https://hg.mozilla.org/mozilla-central/rev/24cf402f2035
https://hg.mozilla.org/mozilla-central/rev/7c9940c57e33
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.