Closed Bug 1412202 Opened 7 years ago Closed 4 years ago

Optimize lexical variables in generator.

Categories

(Core :: JavaScript Engine, enhancement, P1)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement

Tracking

()

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

People

(Reporter: arai, Assigned: jorendorff)

References

(Blocks 3 open bugs)

Details

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

Attachments

(9 files)

Bug 1412202 - Part 1: Merge BaselineCodeGen::emit_InitialYield and emit_Yield. r=jandem
(deleted), text/x-phabricator-request
Details
Bug 1412202 - Part 7: Optimize unaliased generator locals into stack slots. r=jandem
(deleted), text/x-phabricator-request
Details
Bug 1412202 - Part 3: Rename from ExpressionStack to StackStorage in anticipation of including optimized local variables in this array. r=jandem
(deleted), text/x-phabricator-request
Details
Bug 1412202 - Part 4: Copy any unaliased locals between stack and GeneratorObject on suspend/resume. r=jandem
(deleted), text/x-phabricator-request
Details
Bug 1412202 - Part 8: Inhibit the optimization when it would result in too many fixed slots. r=jandem
(deleted), text/x-phabricator-request
Details
Bug 1412202 - Part 9: Blank out slots when leaving a lexical scope (in generators only). r=jandem
(deleted), text/x-phabricator-request
Details
Bug 1412202 - Part 5: Update DebugEnvironments for generator frames. r=jandem
(deleted), text/x-phabricator-request
Details
Bug 1412202 - Part 6: Disable remaining debugger tests that examine generator/async scopes in ways that observe the optimization. r=jandem
(deleted), text/x-phabricator-request
Details
Bug 1412202 - Part 2: Always consider `.generator` to be closed-over. r=jandem
(deleted), text/x-phabricator-request
Details
(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]
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
Regressions: 1684821
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: