Fix Octane-TypeScript regression from bug 1328140

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine: JIT
P1
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

11 months ago
Created attachment 8849169 [details] [diff] [review]
Part 1 - Some small changes

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+
(Assignee)

Comment 2

11 months ago
Created attachment 8849171 [details] [diff] [review]
Part 2 - Some HasTypePropertyId changes

* 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)
(Assignee)

Comment 3

11 months ago
Created attachment 8849172 [details] [diff] [review]
Part 3 - Avoid SetNativeDataProperty type barrier if possible

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+
(Assignee)

Comment 6

11 months ago
(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.

Comment 7

11 months ago
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
(Assignee)

Comment 8

11 months ago
AWFY confirms this fixed the Octane-TypeScript regression. It's a bit faster than before actually.

Comment 9

11 months ago
bugherder
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
Last Resolved: 11 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.