Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2635913002:
[ast] Remove internalization before AST rewriting (Closed)
|
Created:
3 years, 11 months ago by Leszek Swirski Modified:
3 years, 11 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[ast] Remove internalization before AST rewriting
This internalization was not necessary, since the rewriting does not use
the .result name string.
The subsequent internalization is still needed, so to simplify later
refactoring, this CL also adds "releasing" of the disallow scopes and
uses them here immediately before the second internalize. Notably, this
means that the rewriting is now also in the disallow scopes.
Driveby: Remove isolate from the rewriter's processor constructor.
BUG=v8:5832
Review-Url: https://codereview.chromium.org/2635913002
Cr-Commit-Position: refs/heads/master@{#42403}
Committed: https://chromium.googlesource.com/v8/v8/+/bb71555e2e41678897187f3d0c0f42d742865305
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address Ross's comments #Patch Set 3 : Be less stupid #
Messages
Total messages: 25 (13 generated)
The CQ bit was checked by leszeks@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [ast] Remove internalization before AST rewriting This internalization was not necessary, since the rewriting does not use the .result name string. The subsequent internalization is still needed, so to simplify later refactoring, this CL also adds "releasing" of the disallow scopes and uses them here immediately before the second internalize. Notably, this means that the rewriting is now also in the disallow scopes. BUG=v8:5832 ========== to ========== [ast] Remove internalization before AST rewriting This internalization was not necessary, since the rewriting does not use the .result name string. The subsequent internalization is still needed, so to simplify later refactoring, this CL also adds "releasing" of the disallow scopes and uses them here immediately before the second internalize. Notably, this means that the rewriting is now also in the disallow scopes. Driveby: Remove isolate from the rewriter's processor constructor. BUG=v8:5832 ==========
leszeks@chromium.org changed reviewers: + marja@chromium.org, rmcilroy@chromium.org
Hi Marja and Ross, Here's another de-heaping of the AST rewriter, this seems to work in tests and offline discussion with Marja suggests that it makes sense. Ross, I'm including you on this since you added the call originally.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2635913002/diff/1/src/parsing/rewriter.cc File src/parsing/rewriter.cc (right): https://codereview.chromium.org/2635913002/diff/1/src/parsing/rewriter.cc#new... src/parsing/rewriter.cc:378: closure_scope, result, info->ast_value_factory()); It looks like the Processor is only using ast_value_factory for constant strings. After https://codereview.chromium.org/2630343002/ lands could this just take the AstStringConstants instead? https://codereview.chromium.org/2635913002/diff/1/src/parsing/rewriter.cc#new... src/parsing/rewriter.cc:387: info->ast_value_factory()->Internalize(info->isolate()); If we use AstStringConstants above the values will already be internalized, so this internalize here should no longer be necessary, at which point the whole class might be off-thread capable :).
https://codereview.chromium.org/2635913002/diff/1/src/parsing/rewriter.cc File src/parsing/rewriter.cc (right): https://codereview.chromium.org/2635913002/diff/1/src/parsing/rewriter.cc#new... src/parsing/rewriter.cc:378: closure_scope, result, info->ast_value_factory()); On 2017/01/16 14:54:59, rmcilroy wrote: > It looks like the Processor is only using ast_value_factory for constant > strings. After https://codereview.chromium.org/2630343002/ lands could this just > take the AstStringConstants instead? Certainly, do you want me to rebase on top of that and make this patch dependent?
https://codereview.chromium.org/2635913002/diff/1/src/parsing/rewriter.cc File src/parsing/rewriter.cc (right): https://codereview.chromium.org/2635913002/diff/1/src/parsing/rewriter.cc#new... src/parsing/rewriter.cc:378: closure_scope, result, info->ast_value_factory()); On 2017/01/16 15:21:13, Leszek Swirski wrote: > On 2017/01/16 14:54:59, rmcilroy wrote: > > It looks like the Processor is only using ast_value_factory for constant > > strings. After https://codereview.chromium.org/2630343002/ lands could this > just > > take the AstStringConstants instead? > > Certainly, do you want me to rebase on top of that and make this patch > dependent? Yeah that would be good. That CL should land today anyway.
On 2017/01/16 15:35:12, rmcilroy wrote: > https://codereview.chromium.org/2635913002/diff/1/src/parsing/rewriter.cc > File src/parsing/rewriter.cc (right): > > https://codereview.chromium.org/2635913002/diff/1/src/parsing/rewriter.cc#new... > src/parsing/rewriter.cc:378: closure_scope, result, info->ast_value_factory()); > On 2017/01/16 15:21:13, Leszek Swirski wrote: > > On 2017/01/16 14:54:59, rmcilroy wrote: > > > It looks like the Processor is only using ast_value_factory for constant > > > strings. After https://codereview.chromium.org/2630343002/ lands could this > > just > > > take the AstStringConstants instead? > > > > Certainly, do you want me to rebase on top of that and make this patch > > dependent? > > Yeah that would be good. That CL should land today anyway. Ah, nvm, I tried rebasing and there are still issues -- firstly the Processor uses the factory to create various expressions, secondly it creates undefined literals which need internalisation too. Once we push Internalization to bytecode finalization (and wherever it goes for FCG), these won't be a problem, but until then this has to stay as-is.
lgtm
LGTM with a comment. https://codereview.chromium.org/2635913002/diff/1/src/assert-scope.cc File src/assert-scope.cc (right): https://codereview.chromium.org/2635913002/diff/1/src/assert-scope.cc#newcode91 src/assert-scope.cc:91: } Could you just call Release here rather than duplicating code? https://codereview.chromium.org/2635913002/diff/1/src/parsing/rewriter.cc File src/parsing/rewriter.cc (right): https://codereview.chromium.org/2635913002/diff/1/src/parsing/rewriter.cc#new... src/parsing/rewriter.cc:381: DCHECK(ThreadId::Current().Equals(info->isolate()->thread_id())); nit - please add a TODO to remove these releases once internalization is moved to the finalization stage.
The CQ bit was checked by leszeks@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org, marja@chromium.org Link to the patchset: https://codereview.chromium.org/2635913002/#ps20001 (title: "Address Ross's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2635913002/diff/1/src/assert-scope.cc File src/assert-scope.cc (right): https://codereview.chromium.org/2635913002/diff/1/src/assert-scope.cc#newcode91 src/assert-scope.cc:91: } On 2017/01/17 10:41:31, rmcilroy wrote: > Could you just call Release here rather than duplicating code? Not directly, because of the DCHECK, but extracted out into a common function. https://codereview.chromium.org/2635913002/diff/1/src/parsing/rewriter.cc File src/parsing/rewriter.cc (right): https://codereview.chromium.org/2635913002/diff/1/src/parsing/rewriter.cc#new... src/parsing/rewriter.cc:381: DCHECK(ThreadId::Current().Equals(info->isolate()->thread_id())); On 2017/01/17 10:41:32, rmcilroy wrote: > nit - please add a TODO to remove these releases once internalization is moved > to the finalization stage. Done.
The CQ bit was unchecked by leszeks@chromium.org
The CQ bit was checked by leszeks@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org, marja@chromium.org Link to the patchset: https://codereview.chromium.org/2635913002/#ps40001 (title: "Be less stupid")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2635913002/diff/1/src/assert-scope.cc File src/assert-scope.cc (right): https://codereview.chromium.org/2635913002/diff/1/src/assert-scope.cc#newcode91 src/assert-scope.cc:91: } On 2017/01/17 12:11:49, Leszek Swirski wrote: > On 2017/01/17 10:41:31, rmcilroy wrote: > > Could you just call Release here rather than duplicating code? > > Not directly, because of the DCHECK, but extracted out into a common function. Ignore me, I'm an idiot. Done.
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484655425599860,
"parent_rev": "e9981e076acc92709a5a12361d5ab03a9bf5144a", "commit_rev":
"bb71555e2e41678897187f3d0c0f42d742865305"}
Message was sent while issue was closed.
Description was changed from ========== [ast] Remove internalization before AST rewriting This internalization was not necessary, since the rewriting does not use the .result name string. The subsequent internalization is still needed, so to simplify later refactoring, this CL also adds "releasing" of the disallow scopes and uses them here immediately before the second internalize. Notably, this means that the rewriting is now also in the disallow scopes. Driveby: Remove isolate from the rewriter's processor constructor. BUG=v8:5832 ========== to ========== [ast] Remove internalization before AST rewriting This internalization was not necessary, since the rewriting does not use the .result name string. The subsequent internalization is still needed, so to simplify later refactoring, this CL also adds "releasing" of the disallow scopes and uses them here immediately before the second internalize. Notably, this means that the rewriting is now also in the disallow scopes. Driveby: Remove isolate from the rewriter's processor constructor. BUG=v8:5832 Review-Url: https://codereview.chromium.org/2635913002 Cr-Commit-Position: refs/heads/master@{#42403} Committed: https://chromium.googlesource.com/v8/v8/+/bb71555e2e41678897187f3d0c0f42d7428... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/bb71555e2e41678897187f3d0c0f42d7428... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
