Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(30)

Issue 2898163002: Make non-Module generators only context allocate parameters. (Closed)

Created:
3 years, 7 months ago by Jarin
Modified:
3 years, 7 months ago
Reviewers:
neis, adamk, jgruber
CC:
v8-reviews_googlegroups.com, marja+watch_chromium.org
Target Ref:
refs/heads/master
Project:
v8
Visibility:
Public.

Description

Make non-Module generators only context allocate parameters. In particular, local variables should be allocated on stack (in bytecode register), and stored/loaded to the generator object on generator suspend/resume. The CL is based on @adamk's change to scoping/parsers (https://chromium-review.googlesource.com/c/498538/), I only made the debugger cope with this change. I should note that the CL changes the scope type of suspended generators from ScopeType.Closure to ScopeType.Local. In the future we might want to introduce ScopeType.SuspendedGenerator to make the distinction explicit. Some of the changes in the tests have been made because the debugger functions do not return scopes of closed generators anymore. Generators should be allowed to throw away their internal state when they finish. BUG=v8:6368 Review-Url: https://codereview.chromium.org/2898163002 Cr-Commit-Position: refs/heads/master@{#45515} Committed: https://chromium.googlesource.com/v8/v8/+/a957b0f42468d632b09574e3d6719c704eebbfea

Patch Set 1 : Adam's change of scoping for generator locals #

Patch Set 2 : Fix debugger #

Patch Set 3 : Regolden #

Patch Set 4 : Tweak #

Patch Set 5 : Disable tests that assign to optimized variables #

Patch Set 6 : Disable optimization for the debugger test #

Patch Set 7 : Now disable opt in the correct file? #

Patch Set 8 : More flag magic #

Total comments: 4

Patch Set 9 : Fix comment #

Total comments: 24

Patch Set 10 : Address review comments #

Patch Set 11 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2295 lines, -2967 lines) Patch
M src/ast/scopes.h View 2 chunks +8 lines, -0 lines 0 comments Download
M src/ast/scopes.cc View 2 chunks +3 lines, -1 line 0 comments Download
M src/debug/debug-scopes.h View 1 2 3 4 5 6 7 8 9 3 chunks +11 lines, -5 lines 0 comments Download
M src/debug/debug-scopes.cc View 1 2 3 4 5 6 7 8 9 12 chunks +97 lines, -31 lines 0 comments Download
M src/parsing/parser.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +4 lines, -5 lines 0 comments Download
M src/runtime/runtime-debug.cc View 1 2 chunks +10 lines, -0 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/ForAwaitOf.golden View 1 2 3 4 5 6 7 8 9 10 5 chunks +816 lines, -1073 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/ForOfLoop.golden View 1 2 3 4 5 6 7 8 9 10 5 chunks +587 lines, -802 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/Generators.golden View 1 2 3 4 5 6 7 8 9 10 11 chunks +345 lines, -474 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/StandardForLoop.golden View 1 2 3 4 5 6 7 8 9 10 4 chunks +362 lines, -518 lines 0 comments Download
M test/debugger/debug/debug-scopes-suspended-generators.js View 1 2 3 4 5 6 7 8 9 15 chunks +23 lines, -22 lines 0 comments Download
M test/debugger/debug/es8/async-function-debug-evaluate.js View 1 2 3 4 5 6 4 chunks +12 lines, -12 lines 0 comments Download
M test/debugger/debug/es8/async-function-debug-scopes.js View 1 2 3 4 5 6 7 3 chunks +4 lines, -2 lines 0 comments Download
M test/debugger/debug/regress-3225.js View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M test/inspector/debugger/suspended-generator-scopes-expected.txt View 1 4 chunks +12 lines, -12 lines 0 comments Download
M test/inspector/runtime/internal-properties-expected.txt View 1 1 chunk +0 lines, -10 lines 0 comments Download

Messages

Total messages: 56 (43 generated)
Jarin
Could you take a look, please?
3 years, 7 months ago (2017-05-23 14:04:07 UTC) #7
adamk
Thanks for picking this up! Since I wrote the parser/scope bits, I think Georg should ...
3 years, 7 months ago (2017-05-23 20:20:38 UTC) #35
Jarin
https://codereview.chromium.org/2898163002/diff/170001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2898163002/diff/170001/src/parsing/parser.cc#newcode2537 src/parsing/parser.cc:2537: // it minimizes the work needed to suspend and ...
3 years, 7 months ago (2017-05-24 05:03:27 UTC) #36
jgruber
lgtm, thanks! https://codereview.chromium.org/2898163002/diff/180001/src/debug/debug-scopes.cc File src/debug/debug-scopes.cc (right): https://codereview.chromium.org/2898163002/diff/180001/src/debug/debug-scopes.cc#newcode680 src/debug/debug-scopes.cc:680: GetFrame()->SetParameterValue(i, *new_value); Nit: frame->SetParameterValue
3 years, 7 months ago (2017-05-24 08:01:49 UTC) #39
neis
lgtm with minor comments. Haven't looked very carefully at the debug-scopes tests.
3 years, 7 months ago (2017-05-24 10:39:12 UTC) #40
neis
https://codereview.chromium.org/2898163002/diff/180001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2898163002/diff/180001/src/ast/scopes.cc#newcode2198 src/ast/scopes.cc:2198: MustAllocateInContext(var)) { This should be moved into MustAllocateInContext(var). Or ...
3 years, 7 months ago (2017-05-24 10:39:37 UTC) #41
Jarin
I fixed all the debugger-related comments, but I have no idea about the scoping/parser part, ...
3 years, 7 months ago (2017-05-24 12:32:21 UTC) #42
adamk
https://codereview.chromium.org/2898163002/diff/180001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2898163002/diff/180001/src/ast/scopes.cc#newcode2198 src/ast/scopes.cc:2198: MustAllocateInContext(var)) { On 2017/05/24 10:39:36, neis wrote: > This ...
3 years, 7 months ago (2017-05-24 13:04:00 UTC) #47
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2898163002/220001
3 years, 7 months ago (2017-05-24 13:07:48 UTC) #50
commit-bot: I haz the power
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/v8/v8/+/a957b0f42468d632b09574e3d6719c704eebbfea
3 years, 7 months ago (2017-05-24 13:55:06 UTC) #53
neis
https://codereview.chromium.org/2898163002/diff/180001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2898163002/diff/180001/src/parsing/parser.cc#newcode709 src/parsing/parser.cc:709: scope->ForceContextAllocation(); On 2017/05/24 13:04:00, adamk wrote: > On 2017/05/24 ...
3 years, 7 months ago (2017-05-24 13:59:46 UTC) #54
adamk
https://codereview.chromium.org/2898163002/diff/180001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2898163002/diff/180001/src/parsing/parser.cc#newcode709 src/parsing/parser.cc:709: scope->ForceContextAllocation(); On 2017/05/24 13:59:46, neis wrote: > On 2017/05/24 ...
3 years, 7 months ago (2017-05-24 14:31:23 UTC) #55
Michael Achenbach
3 years, 7 months ago (2017-05-25 17:38:40 UTC) #56
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:220001) has been created in
https://codereview.chromium.org/2902423002/ by machenbach@chromium.org.

The reason for reverting is: Speculative revert for layout test changes:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui...

Looks like this might have caused the change in:
inspector/sources/debugger-ui/function-generator-details.html

See also:
https://github.com/v8/v8/wiki/Blink-layout-tests.

Powered by Google App Engine
This is Rietveld 408576698