Babel class inheritance code confuses TI and Ion inlining

RESOLVED FIXED in Firefox 55

Status

()

P1
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 2 bugs, {perf})

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
Ember's Glimmer VM is transpiled with Babel. For derived classes, Babel uses setPrototypeOf on the constructor function. This pattern confuses TI and hence we don't inline some trivial base constructors and end up with megamorphic property accesses on the Ember benchmark in bug 1352486.

I have patches to fix this and some related issues with proto mutation,
(Assignee)

Comment 1

2 years ago
Created attachment 8859509 [details] [diff] [review]
Part 1 - Allow inlining functions with unknown properties

Ion is currently unable to inline functions with unknown properties, because we need to add a type constraint to trigger invalidation of compilations that inlined the script when the callees stack types change.

This patch replaces this constraint with a Vector of RecompileInfo entries (compilations that inlined this script) on TypeScript, so we can invalidate them on type changes.
Attachment #8859509 - Flags: review?(bhackett1024)
(Assignee)

Updated

2 years ago
Blocks: 1307062
Keywords: perf
Priority: -- → P1
Attachment #8859509 - Flags: review?(bhackett1024) → review+

Comment 2

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a446a56598f
part 1 - Track Ion-inlined scripts explicitly so we can inline functions with unknown properties. r=bhackett
(Assignee)

Updated

2 years ago
Keywords: leave-open

Comment 3

2 years ago
sorry had to back this out for leak issue like https://treeherder.mozilla.org/logviewer.html#?job_id=93241968&repo=mozilla-inbound&lineNumber=9557
Flags: needinfo?(jdemooij)
(Assignee)

Comment 4

2 years ago
Ugh a js_free must be js_delete, I had that change at some point but must have dropped it.

Comment 5

2 years ago
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ccc9c855766
Backed out changeset 7a446a56598f for leak issue

Comment 6

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/954eeda43262
part 1 - Track Ion-inlined scripts explicitly so we can inline functions with unknown properties. r=bhackett
(Assignee)

Comment 8

2 years ago
Relanded and stuck.
Flags: needinfo?(jdemooij)
(Assignee)

Comment 9

2 years ago
Created attachment 8860903 [details] [diff] [review]
Part 2 - Remove unnecessary setNewGroupUnknown in SetClassAndProto

This patch is also unlikely to improve anything but it's another incremental step.
Attachment #8860903 - Flags: review?(bhackett1024)
Attachment #8860903 - Flags: review?(bhackett1024) → review+

Comment 10

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c35f0ad34397
part 2 - Remove unnecessary setNewGroupUnknown call in SetClassAndProto. r=bhackett
(Assignee)

Comment 12

2 years ago
Created attachment 8863358 [details] [diff] [review]
Part 3 - TI changes

This patch makes 2 changes:

(1) When we mutate the prototype of a native object, instead of marking the new group as having unknown properties, we propagate the property types from the object to the new group.

(2) When we mutate the prototype of a scripted function, we create a new group and propagate the canonical JSFunction to it.

On the Ember benchmark we can inline more functions now and I hope these changes will make us more robust in a lot of other cases that involve proto mutation.
Attachment #8863358 - Flags: review?(bhackett1024)
(Assignee)

Comment 13

2 years ago
Comment on attachment 8863358 [details] [diff] [review]
Part 3 - TI changes

Brian has no internet access for a while but reviewed this over email.
Attachment #8863358 - Flags: review?(bhackett1024) → review+

Comment 14

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6315d186b4b
part 3 - Don't mark the new group as having unknown properties when changing an object's proto. r=bhackett
(Assignee)

Updated

2 years ago
Keywords: leave-open

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b6315d186b4b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1367437
Depends on: 1368570
This will affect speedometer with updated Ember. marking [qf:p1] for tracking high impact improvements.
Whiteboard: [qf:p1]
Depends on: 1395919
You need to log in before you can comment on or make changes to this bug.