1341937 - Improve Ion performance of LexicalEnvironmentObjects

Status: RESOLVED FIXED

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

Description

[Ted Campbell [:tcampbell]]

Continuing on Bug 1273858, we need to improve performance.

1) Currently VMCalls are used to implement this but should largely be inlined as mash.createGCObject calls

2) Explore better analysis of whether the environment is needed. The parser does minimal work to determine this, but Ion doesn't do anything for these environments.

Comment 1

[Nicolas B. Pierron [:nbp]]

We should also consider adding it to the escape analysis and recover instructions, such that we can avoid any allocation if none of their uses are escaped.

Note, adding it as part of the recover instruction would imply splitting the instruction in 2, such that one can hold the GC pointers in the snapshot, while the other does the create operation, as is done for NewObject and the templateObject already.

Comment 2

[Ted Campbell [:tcampbell]]

Baseline also uses a VMCall here and should be improved. The baseline affects ARES-6 benchmark. Perhaps we need a new/reused IC?

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Ted Campbell [:tcampbell] from comment #2)
> Baseline also uses a VMCall here and should be improved. The baseline
> affects ARES-6 benchmark.

Just curious, is that because we're not Ion-compiling some scripts?

Comment 4

[Tom S [:evilpie]]

Yes, ARES-6 spends a lot of time in generator functions that we don't ion compile.

Comment 5

[André Bargull [:anba]]

Attachment: Bug 1341937 - Part 1: Include slot numbers when printing opcodes for fixed slot instructions. r=jandem!

Also change the format M{Load,Store}DynamicSlot to append the slot information
at the end, because it was confusing to see a bare number in the middle of the
output for MStoreDynamicSlot. And change MStoreDynamicSlot to print the type
information of its operands.

Comment 6

[André Bargull [:anba]]

Attachment: Bug 1341937 - Part 2: Add scope info to {Recreate,Freshen}LexicalEnv. r=jandem!

Later patches will use the scope to retrieve the environment shape. Also assert
in the interpreter that the scope matches the current scope.

Depends on D156777

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1341937 - Part 3: Generate MNewLexicalEnvironmentObject definition. r=jandem!

Add LexicalScope as a GC pointer type and then generate the code
for MNewLexicalEnvironmentObject.

Depends on D156778

Comment 8

[André Bargull [:anba]]

Attachment: Bug 1341937 - Part 4: Add Warp snapshots for environment objects. r=jandem!

Add Warp snapshots which store the template object information for the
environment objects.

Depends on D156779

Comment 9

[André Bargull [:anba]]

Attachment: Bug 1341937 - Part 5: Retrieve the scope from the warp snapshot. r=jandem!

Use the Warp snapshots to retrieve the environment scopes.

Depends on D156780

Comment 10

[André Bargull [:anba]]

Attachment: Bug 1341937 - Part 6: Retrieve the scope from the template object. r=jandem!

Store the template object in the environment instruction and later retrieve
the scope from the template object.

Depends on D156781

Comment 11

[André Bargull [:anba]]

Attachment: Bug 1341937 - Part 7: Store the environment slot in a separate instruction. r=jandem!

This matches MNewCallObject and should help if we ever support scalar
replacement of lexical environment objects.

Depends on D156782

Comment 12

[André Bargull [:anba]]

Attachment: Bug 1341937 - Part 8: Inline allocation of environment objects using createGCObject. r=jandem!

Everything is now in place to allow inline allocation of environment objects.

Depends on D156783

Comment 13

[André Bargull [:anba]]

Attachment: Bug 1341937 - Part 9: Inline buildCopyLexicalEnvOp. r=jandem!

Inline the method in preparation for the next two parts.

Depends on D156784

Comment 14

[André Bargull [:anba]]

Attachment: Bug 1341937 - Part 10: Use MNewLexicalEnvironmentObject for RecreateLexicalEnv. r=jandem!

Use MNewLexicalEnvironmentObject to inline the allocation.

Depends on D156785

Comment 15

[André Bargull [:anba]]

Attachment: Bug 1341937 - Part 11: Use MNewLexicalEnvironmentObject for FreshenLexicalEnv. r=jandem!

Use MNewLexicalEnvironmentObject to inline the allocation and then copy all
slots manually.

Depends on D156786

Comment 16

[André Bargull [:anba]]

Attachment: Bug 1341937 - Part 12: Remove CopyLexicalEnvironmentObject. r=jandem!

No longer needed after the last two parts.

Depends on D156787

Comment 17

[André Bargull [:anba]]

Attachment: Bug 1341937 - Part 13: Check eliding post write barriers is correct. r=jandem!

Add a debug-only check that it's okay to elide the post write barriers.

Depends on D156788

Comment 18

[André Bargull [:anba]]

Attachment: Bug 1341937 - Part 14: Remove a misleading comment. r=jandem!

The template object is always present, so a comment mentioning it's only
conditionally present is a bit misleading.

Depends on D156789

Comment 19

[André Bargull [:anba]]

Attachment: Bug 1341937 - Part 15: Rename environment object create methods. r=jandem!

• Use createTemplateObject iff creating a template object. This implies the
  object is tenured, so always pass gc::TenuredHeap.
• Add createWithoutEnclosing for Warp to create an environment object without
  an environment oject. The object is never pre-tenured, so always use
  gc::DefaultHeap.
• No public method now accepts a gc::InitialHeap parameter.

Comment 20

[André Bargull [:anba]]

Attachment: Bug 1341937 - Part 16: Remove extra barrier in CallObject::create ool-path. r=jandem!

Depends on D156801

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f78fd4a216da
Part 1: Include slot numbers when printing opcodes for fixed slot instructions. r=jandem
https://hg.mozilla.org/integration/autoland/rev/599ac6a9d028
Part 2: Add scope info to {Recreate,Freshen}LexicalEnv. r=jandem
https://hg.mozilla.org/integration/autoland/rev/35754caa8eec
Part 3: Generate MNewLexicalEnvironmentObject definition. r=jandem
https://hg.mozilla.org/integration/autoland/rev/9cf7225b2029
Part 4: Add Warp snapshots for environment objects. r=jandem
https://hg.mozilla.org/integration/autoland/rev/2936cd579655
Part 5: Retrieve the scope from the warp snapshot. r=jandem
https://hg.mozilla.org/integration/autoland/rev/6e471b207c2f
Part 6: Retrieve the scope from the template object. r=jandem
https://hg.mozilla.org/integration/autoland/rev/96e9152c2cfe
Part 7: Store the environment slot in a separate instruction. r=jandem
https://hg.mozilla.org/integration/autoland/rev/90917af9e65e
Part 8: Inline allocation of environment objects using createGCObject. r=jandem
https://hg.mozilla.org/integration/autoland/rev/77b20f516feb
Part 9: Inline buildCopyLexicalEnvOp. r=jandem
https://hg.mozilla.org/integration/autoland/rev/8afd8f0c47d9
Part 10: Use MNewLexicalEnvironmentObject for RecreateLexicalEnv. r=jandem
https://hg.mozilla.org/integration/autoland/rev/51c8183e8ac2
Part 11: Use MNewLexicalEnvironmentObject for FreshenLexicalEnv. r=jandem
https://hg.mozilla.org/integration/autoland/rev/e7f4d971689d
Part 12: Remove CopyLexicalEnvironmentObject. r=jandem
https://hg.mozilla.org/integration/autoland/rev/d385628a29e6
Part 13: Check eliding post write barriers is correct. r=jandem
https://hg.mozilla.org/integration/autoland/rev/81dbd17dab44
Part 14: Remove a misleading comment. r=jandem
https://hg.mozilla.org/integration/autoland/rev/48520b1aa12d
Part 15: Rename environment object create methods. r=jandem
https://hg.mozilla.org/integration/autoland/rev/5da07fdf2c5e
Part 16: Remove extra barrier in CallObject::create ool-path. r=jandem

Comment 22

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/f78fd4a216da
https://hg.mozilla.org/mozilla-central/rev/599ac6a9d028
https://hg.mozilla.org/mozilla-central/rev/35754caa8eec
https://hg.mozilla.org/mozilla-central/rev/9cf7225b2029
https://hg.mozilla.org/mozilla-central/rev/2936cd579655
https://hg.mozilla.org/mozilla-central/rev/6e471b207c2f
https://hg.mozilla.org/mozilla-central/rev/96e9152c2cfe
https://hg.mozilla.org/mozilla-central/rev/90917af9e65e
https://hg.mozilla.org/mozilla-central/rev/77b20f516feb
https://hg.mozilla.org/mozilla-central/rev/8afd8f0c47d9
https://hg.mozilla.org/mozilla-central/rev/51c8183e8ac2
https://hg.mozilla.org/mozilla-central/rev/e7f4d971689d
https://hg.mozilla.org/mozilla-central/rev/d385628a29e6
https://hg.mozilla.org/mozilla-central/rev/81dbd17dab44
https://hg.mozilla.org/mozilla-central/rev/48520b1aa12d
https://hg.mozilla.org/mozilla-central/rev/5da07fdf2c5e

Comment 23

[André Bargull [:anba]]

(In reply to Nicolas B. Pierron [:nbp] from comment #1)

We should also consider adding it to the escape analysis and recover
instructions, such that we can avoid any allocation if none of their uses
are escaped.

This is tracked in bug 1700382.

Note, adding it as part of the recover instruction would imply splitting the
instruction in 2, such that one can hold the GC pointers in the snapshot,
while the other does the create operation, as is done for NewObject and the
templateObject already.

The patch series for this bug already split the instruction in preparation for possible scalar replacement. The biggest obstacle right now is to somehow recover the scope chain on bailout. We need something like jit::EnsureHasEnvironmentObjects, but instead of only recovering the CallObject, we need to recover the whole scope chain.

Comment 24

[Nicolas B. Pierron [:nbp]]

(In reply to André Bargull [:anba] from comment #23)

The patch series for this bug already split the instruction in preparation for possible scalar replacement. The biggest obstacle right now is to somehow recover the scope chain on bailout.

Everything should already in place for doing it, as we already support recovering function environment.

Function environment were a bit more tricky due to observable flags from arguments.caller, which causes eager bailout when a value is queried but was never materialized by Ion.

Comment 25

[André Bargull [:anba]]

In a case like this (Surrounding caller code omitted):

function f() {
  {
    let x = 0;
    return () => x;
  }
}

JSScript::needsBodyEnvironment() returns true, which leads to returning ObservableNotRecoverable in CompileInfo::getSlotObservableKind(). IsObjectEscaped rejects scalar replacement in this case. (See also bug 1342483.)

I assume changing EnsureHasEnvironmentObjects to recover the whole scope chain could help here.

Comment 26

[André Bargull [:anba]]

(Comment update fail.)

Comment 27

[Nicolas B. Pierron [:nbp]]

(In reply to André Bargull [:anba] from comment #25)

function f() {
  {
    let x = 0;
    return () => x;
  }
}

Well in this case x is escaped via the arrow function which captures it. Scalar Replacement should only be capable of removing it if f is inline, as well as the caller of the result of f.

Comment 28

[André Bargull [:anba]]

Yes, I've omitted the surrounding caller code which calls f and the arrow function in the example.

Comment 29

[Jon Coppeard (:jonco)]

(In reply to André Bargull [:anba] from comment #20)

Created attachment 9293662 [details]
Bug 1341937 - Part 16: Remove extra barrier in CallObject::create ool-path. r=jandem!
Is there some justification for why this barrier is not needed? In general the GC can allocate objects in the tenured heap sometimes even when nursery allocation is requested.

Comment 30

[André Bargull [:anba]]

Part 16 was added based on the review comments from part 7: Before this patch stack, CallObject was the only environment object which performed an explicit barrier in the OOL path. Part 16 aligned it with other environment objects, like for example NamedLambdaObject, which don't perform an extra barrier.

In Part 13, I've also added extra assertions to check it's okay to elide these barriers. And there's bug 1622192, created in response to this review, to further clean-up these implicit allocation invariants.