1898270 - Pretenure Lambda allocations

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

Bug 1894012 comment 12 suggests that this could be helpful for speedometer3.

Comment 1

[Jon Coppeard (:jonco)]

I wrote a patch for this, although I no longer think that this will help sp3 much since most nursery collection does not happen in the timed part of the benchmark any more.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: WIP: Bug 1898270 - Pretenure lambda allocations

Comment 3

[Jon Coppeard (:jonco)]

The patch is out of date since bug 1937570, but that should make it simpler to add this.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1898270 - Part 1: Transpile NewFunctionCloneResult cache IR r?jandem

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1898270 - Part 2: Pretenure lambda allocations by adding an allocation site for NewFunctionCloneResult r?jandem

Comment 6

[Jon Coppeard (:jonco)]

I tried to do this by transpiling the NewFunctionCloneResult cache IR and then giving it an allocation site, similar to how it's done for e.g. NewPlainObject result. However the first patch causes assertion failures:

Assertion failure: forIns->bailoutKind() != BailoutKind::TranspiledCacheIR, at /home/jon/clone/memento/js/src/jit/TypePolicy.cpp:62

Jan if you have time could you take a look? It's probably something obvious. Maybe related to the fact that MLambda has a different type policy... I don't know what this means though.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Jon Coppeard (:jonco) from comment #6)

Jan if you have time could you take a look? It's probably something obvious. Maybe related to the fact that MLambda has a different type policy... I don't know what this means though.

I had a quick look. In some edge cases the environment chain has type Value instead of Object and this confuses the type policy code where we assert that transpiled CacheIR has the correct MIR types.

To fix this you can add something like this to WarpCacheIRTranspiler::emitNewFunctionCloneResult to force the operand type to be an object:

  MDefinition* env = currentBlock()->environmentChain();

  // The environment chain must be an object, but the MIR type can be Value when
  // phis are involved.
  if (env->type() != MIRType::Object) {
    MOZ_ASSERT(env->type() == MIRType::Value);
    auto* unbox = MUnbox::New(alloc(), env, MIRType::Object,
                              MUnbox::Infallible);
    current->add(unbox);
    env = unbox;
  }

I want to look into this a bit more because I'm curious to know why our MIR type analysis fails to know that this is always an object, but that investigation doesn't need to block your work.

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #7)

I want to look into this a bit more because I'm curious to know why our MIR type analysis fails to know that this is always an object, but that investigation doesn't need to block your work.

I looked into this a bit. It's caused by an MUnreachableResult instruction inserted in NewFakeLoopPredecessor that has MIRType::Value . Maybe Phi type analysis should ignore these operands because they're unreachable, but that's complicated enough that it's probably not worth doing until we see it show up as an actual performance problem.

Comment 9

[Jon Coppeard (:jonco)]

(In reply to Jan de Mooij [:jandem] from comment #7)
Thanks for looking into this. Your suggestion does fix the test failures.

The current status is that this now pretenures the lambda allocations. However it only helps the test case a little because CallObject allocations are not pretenured.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd63027fecb0
Part 1: Transpile NewFunctionCloneResult cache IR r=jandem
https://hg.mozilla.org/integration/autoland/rev/d3513d462145
Part 2: Pretenure lambda allocations by adding an allocation site for NewFunctionCloneResult r=jandem

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/cd63027fecb0
https://hg.mozilla.org/mozilla-central/rev/d3513d462145

Comment 12

[Acasandrei Beatrice (needinfo me)]

(In reply to Pulsebot from comment #10)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd63027fecb0
Part 1: Transpile NewFunctionCloneResult cache IR r=jandem
https://hg.mozilla.org/integration/autoland/rev/d3513d462145
Part 2: Pretenure lambda allocations by adding an allocation site for
NewFunctionCloneResult r=jandem

Perfherder has detected a devtools performance change from push d3513d46214551f781ba0269eb09bee49339639f.

If you have any questions, please reach out to a performance sheriff. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
7%	damp jstracer.profiler-recording-performance.DAMP	windows11-64-24h2-shippable	e10s fission stylo webrender	149.63 -> 138.57

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a performance sheriff to do that for you.

You can run all of these tests on try with ./mach try perf --alert 45029

The following documentation link provides more information about this command.