Closed Bug 1412202 Opened 3 years ago Closed 2 months ago

Optimize lexical variables in generator.

Categories

(Core :: JavaScript Engine, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
83 Branch
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox83 --- fixed

People

(Reporter: arai, Assigned: jorendorff)

References

(Blocks 4 open bugs, Regressed 1 open bug)

Details

(Keywords: perf-alert, Whiteboard: [MemShrink:P2])

Attachments

(9 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
(In reply to Ted Campbell [:tcampbell] (PTO until Oct 30) from bug 1393712 comment #2)
> A big part of this is when have generators, we turn all bindings into heap
> allocated instead of using the frame. This is done to avoid copying stack on
> every yield. This is a problem when there are lets in a loop we are
> allocating a new lexical environment each iteration. I'm in the process of
> looking at a solution for this. One one of the ARES-6 benchmarks, something
> like 40% of runtime is eliminated by turning the let into a var.

code:
  dis(function() { let x = 10; })
  dis(function*() { let x = 10; })

actual result:
  pushlexicalenv is emitted only in generator case.
Blocks: ares-6
Priority: -- → P3
handling lexical environment is taking too much time in ARES-6 Basic.
it's taking ~20% of total execution time for single run.
Blocks: 1521435
Blocks: 1542660

I'm marking this as MemShrink because bug 1542660 shows that a failure to optimize can cause leaks.

Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P2]
Blocks: 1498485
Assignee: nobody → jorendorff
Priority: P3 → P1

This fixes bug 1542660 for the usual case (no direct eval, less than
ParseContext::GeneratorFixedSlotLimit locals), so this adds a unit test
contributed by Mathieu Hofman in that bug.

Depends on D93385

Without these changes, the tests fail because the engine returns
{optimizedOut: true} for some uses of frame.environment.getVariable and
frame.this. The right solution probably involves teaching
DebugEnvironmentProxyHandler::handleUnaliasedAccess how to access suspended
GeneratorObject state.

Depends on D93387

Regressions: 1671391

No immediate effect, but when we start optimizing generator locals into stack
slots later in this stack, we do not want to optimize .generator, as e.g.
js::GetGeneratorObjectForFrame assumes it is stored in the CallObject.

Before this patch, there was no way in the frontend to force binding to be
closed-over.

Previously reviewed by jandem as part of D93381.

Depends on D93381

Attachment #9181332 - Attachment description: Bug 1412202 - Part 3: Rename from ExpressionStack to StackStorage in anticipation of including optimized local variables in this array, at least for a time. r?jandem → Bug 1412202 - Part 3: Rename from ExpressionStack to StackStorage in anticipation of including optimized local variables in this array. r?jandem
Attachment #9181333 - Attachment description: Bug 1412202 - Part 4: Treat unaliased locals like the expression stack: copy between stack and GeneratorObject on suspend/resume. r?jandem → Bug 1412202 - Part 4: Copy any unaliased locals between stack and GeneratorObject on suspend/resume. r=jandem
Attachment #9181336 - Attachment description: Bug 1412202 - Part 7: Update DebugEnvironments for generator frames. r?jandem → Bug 1412202 - Part 5: Update DebugEnvironments for generator frames. r?jandem
Attachment #9181337 - Attachment description: Bug 1412202 - Part 8: Disable remaining debugger tests that examine generator/async scopes in ways that observe the optimization. r?jandem → Bug 1412202 - Part 6: Disable remaining debugger tests that examine generator/async scopes in ways that observe the optimization. r?jandem
Attachment #9181331 - Attachment description: Bug 1412202 - Part 2: Change frontend to make it possible for generator local bindings to be non-"aliased". r=jandem → Bug 1412202 - Part 7: Optimize unaliased generator locals into stack slots. r=jandem
Attachment #9181334 - Attachment description: Bug 1412202 - Part 5: Inhibit the optimization when it would result in too many fixed slots. r?jandem → Bug 1412202 - Part 8: Inhibit the optimization when it would result in too many fixed slots. r?jandem
Attachment #9181335 - Attachment description: Bug 1412202 - Part 6: Blank out slots when leaving a lexical scope (in generators only). r?jandem → Bug 1412202 - Part 9: Blank out slots when leaving a lexical scope (in generators only). r?jandem
Pushed by jorendorff@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4ff48b83bad6
Part 1: Merge BaselineCodeGen::emit_InitialYield and emit_Yield. r=jandem
https://hg.mozilla.org/integration/autoland/rev/3909f706c022
Part 2: Always consider `.generator` to be closed-over. .
https://hg.mozilla.org/integration/autoland/rev/c1f5bdf4ca3c
Part 3: Rename from ExpressionStack to StackStorage in anticipation of including optimized local variables in this array. r=jandem
https://hg.mozilla.org/integration/autoland/rev/62490c81ec8c
Part 4: Copy any unaliased locals between stack and GeneratorObject on suspend/resume. r=jandem
https://hg.mozilla.org/integration/autoland/rev/71f894279ce5
Part 5: Update DebugEnvironments for generator frames. r=jandem
https://hg.mozilla.org/integration/autoland/rev/3b4683d1d783
Part 6: Disable remaining debugger tests that examine generator/async scopes in ways that observe the optimization. r=jandem
https://hg.mozilla.org/integration/autoland/rev/50ff9b1a922a
Part 7: Optimize unaliased generator locals into stack slots. r=jandem
https://hg.mozilla.org/integration/autoland/rev/6459dd328f07
Part 8: Inhibit the optimization when it would result in too many fixed slots. r=jandem
https://hg.mozilla.org/integration/autoland/rev/8127ab469fc8
Part 9: Blank out slots when leaving a lexical scope (in generators only). r=jandem
Attachment #9181329 - Attachment description: Bug 1412202 - Part 1: Merge BaselineCodeGen::emit_InitialYield and emit_Yield. r?jandem → Bug 1412202 - Part 1: Merge BaselineCodeGen::emit_InitialYield and emit_Yield. r=jandem
Attachment #9181777 - Attachment description: Bug 1412202 - Part 2: Always consider `.generator` to be closed-over. r?jandem. → Bug 1412202 - Part 2: Always consider `.generator` to be closed-over. r=jandem
Attachment #9181332 - Attachment description: Bug 1412202 - Part 3: Rename from ExpressionStack to StackStorage in anticipation of including optimized local variables in this array. r?jandem → Bug 1412202 - Part 3: Rename from ExpressionStack to StackStorage in anticipation of including optimized local variables in this array. r=jandem
Attachment #9181336 - Attachment description: Bug 1412202 - Part 5: Update DebugEnvironments for generator frames. r?jandem → Bug 1412202 - Part 5: Update DebugEnvironments for generator frames. r=jandem
Attachment #9181337 - Attachment description: Bug 1412202 - Part 6: Disable remaining debugger tests that examine generator/async scopes in ways that observe the optimization. r?jandem → Bug 1412202 - Part 6: Disable remaining debugger tests that examine generator/async scopes in ways that observe the optimization. r=jandem
Attachment #9181334 - Attachment description: Bug 1412202 - Part 8: Inhibit the optimization when it would result in too many fixed slots. r?jandem → Bug 1412202 - Part 8: Inhibit the optimization when it would result in too many fixed slots. r=jandem
Attachment #9181335 - Attachment description: Bug 1412202 - Part 9: Blank out slots when leaving a lexical scope (in generators only). r?jandem → Bug 1412202 - Part 9: Blank out slots when leaving a lexical scope (in generators only). r=jandem
Pushed by jorendorff@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f98859c9c6ca
Part 1: Merge BaselineCodeGen::emit_InitialYield and emit_Yield. r=jandem
https://hg.mozilla.org/integration/autoland/rev/21ec34c72ce7
Part 2: Always consider `.generator` to be closed-over.
https://hg.mozilla.org/integration/autoland/rev/edd866af16c2
Part 3: Rename from ExpressionStack to StackStorage in anticipation of including optimized local variables in this array. r=jandem
https://hg.mozilla.org/integration/autoland/rev/dac2bfed4348
Part 4: Copy any unaliased locals between stack and GeneratorObject on suspend/resume. r=jandem
https://hg.mozilla.org/integration/autoland/rev/a3782500d573
Part 5: Update DebugEnvironments for generator frames. r=jandem
https://hg.mozilla.org/integration/autoland/rev/4b39f862e0e8
Part 6: Disable remaining debugger tests that examine generator/async scopes in ways that observe the optimization. r=jandem
https://hg.mozilla.org/integration/autoland/rev/191c45160e4c
Part 7: Optimize unaliased generator locals into stack slots. r=jandem
https://hg.mozilla.org/integration/autoland/rev/e87f9a52024c
Part 8: Inhibit the optimization when it would result in too many fixed slots. r=jandem
https://hg.mozilla.org/integration/autoland/rev/3a1b79f753e2
Part 9: Blank out slots when leaving a lexical scope (in generators only). r=jandem

== Change summary for alert #27254 (as of Sun, 18 Oct 2020 03:44:26 GMT) ==

Improvements:

Ratio Suite Test Platform Options Absolute values (old vs new)
8% raptor-ares6-firefox macosx1014-64-shippable 67.71 -> 62.02
7% raptor-ares6-firefox linux64-shippable 58.56 -> 54.27
7% raptor-ares6-firefox windows10-64-shippable-qr webrender 63.50 -> 58.97
7% raptor-ares6-firefox windows10-64-shippable 63.21 -> 58.83
6% raptor-ares6-firefox windows7-32-shippable 63.59 -> 59.66
6% raptor-ares6-firefox linux64-shippable-qr webrender 57.92 -> 54.40

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=27259

Keywords: perf-alert
Flags: needinfo?(jorendorff)
Regressions: 1673080
Regressions: 1671762
You need to log in before you can comment on or make changes to this bug.