Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2318653002:
[Interpreter] Localize the CanonicalHandleScope to parsing and renumbering. (Closed)
Description[Interpreter] Localize the CanonicalHandleScope to parsing and renumbering.
Ignition requires that objects which will be inserted into the
constant pool are canonicalized (to enable off-thread bytecode
generation). We created a CanonicalizeHandleScope across parse/compile
however this impacts performance (~5-8% on CodeLoad).
Now we localize the CanonicalHandleScope to only the parse /
internalization and renumbering phases where objects are created which
could end up in the constant array pool. This seems to address
the performance regression.
BUG=v8:5203, chromium:634953
Committed: https://crrev.com/b37daacd6b94086ad63ab763587ab2a7b8c88c6b
Cr-Commit-Position: refs/heads/master@{#39443}
Patch Set 1 #Patch Set 2 #
Total comments: 1
Patch Set 3 : Move scopes back to compiler.cc #Patch Set 4 : Rebase #
Messages
Total messages: 30 (18 generated)
The CQ bit was checked by rmcilroy@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
Description was changed from ========== [Interpreter] Move CanonicalHandleScopes for Ignition to limit overhead. This moves the CanonicalHandleScopes to only those parts of the pipeline where they are required - ast internalization and ast numbering. This reduces the overhead of the CanonicalHandleScopes by more than 50%. BUG=chromium:634953 ========== to ========== [Interpreter] Localize the CanonicalHandleScope to ast internalization and renumbering. Ignition requires that objects which will be inserted into the constant pool are canonicalized (to enable off-thread bytecode generation). We created a CanonicalizeHandleScope across parse/compile however this impacts performance (~5-8% on CodeLoad). Now that AST internalization is no longer done on-the-fly, we can localize the CanonicalHandleScope to only the internalization and renumbering phases where objects are created which could end up in the constant array pool. This seems to address the performance regression. BUG=v8:5203,chromium:634953 ==========
rmcilroy@chromium.org changed reviewers: + marja@chromium.org
Marja, this depends on the previous two CLs (plus Jochen's). It gets rid of a perf regression on CodeLoad for Ignition which happened when the CanonicalHandleScope was originally introduced. It does mean the ast-value-factory needs to know whether ignition is enabled, but it moves the scope much closer to where the handles are created, and avoids having to create the scopes in a number of different functions which call Parser.Internalize().
Hmm, I don't get this... why does this get rid of the performance regression? It shouldn't matter at what level the canonical handle scope is created, if all created handles go inside it anyway. But apparently it matters, so clearly I'm missing something! :) https://codereview.chromium.org/2318653002/diff/20001/src/ast/ast-numbering.cc File src/ast/ast-numbering.cc (right): https://codereview.chromium.org/2318653002/diff/20001/src/ast/ast-numbering.c... src/ast/ast-numbering.cc:584: if (FLAG_ignition) canonical.reset(new CanonicalHandleScope(isolate_)); Why do we need to create the canonical handle scope here? What internalizes below?
On 2016/09/12 07:26:41, marja wrote: > Hmm, I don't get this... why does this get rid of the performance regression? It > shouldn't matter at what level the canonical handle scope is created, if all > created handles go inside it anyway. But apparently it matters, so clearly I'm > missing something! :) As far as I can tell it's due to other handles being created in other (non-parser related) parts of the compilation. These never get put into the constant pool, so don't need to be canoncicalized. I also think this is a bit cleaner (only two scopes rather than a number of scopes all trying to capture the same internalization point). > https://codereview.chromium.org/2318653002/diff/20001/src/ast/ast-numbering.cc > File src/ast/ast-numbering.cc (right): > > https://codereview.chromium.org/2318653002/diff/20001/src/ast/ast-numbering.c... > src/ast/ast-numbering.cc:584: if (FLAG_ignition) canonical.reset(new > CanonicalHandleScope(isolate_)); > Why do we need to create the canonical handle scope here? What internalizes > below? The BuildConstantProperties in VisitObjectLiteral and VisitArrayLiteral both create heap objects which are put into the constant pool. It would be nice if we could do this at some other point (e.g., during ast-internalization) since it would avoid the ast-numbering phase having to be run on-heap, but it would involve having to add array value types to the ast-value-factory and a bit of refactoring in how the constant properties are built.
lgtm Routing some offline discussion here: - The efficiency problem is that the CanonicalHandleScope is still alive during compilation. - It's not easy to just .reset the unique_ptr at the right moment, because we need to have the CanonicalHandleScope for the ast numbering, and not have it for compilation, and those two are done by the same function. - I'm not a fan of the design where AstValueFactory needs to know about Ignition, I still think it's a wrong abstraction layer for it. compiler.cc is the file which anyway knows about ignitioning (and makes the decision on whether to use it or not) so the CanonicalHandleScope would've been more suitable there. - However, I don't have an immediate alternative in mind which would make the design cleaner :/ - Toyed with some alternatives: -- passing a bool "need_canonical_handle_scope" all the way down to the places which would then create it in this CL -- passing the CanonicalHandleScope unique_ptr around to the function which has the right place for resetting it after ast numbering but before compilation but none of them seemed very attractive.
jochen@chromium.org changed reviewers: + jochen@chromium.org
wouldn't it be enough to create the handle scopes closer to the calls to renumber / analyze in compiler.cc?
Description was changed from ========== [Interpreter] Localize the CanonicalHandleScope to ast internalization and renumbering. Ignition requires that objects which will be inserted into the constant pool are canonicalized (to enable off-thread bytecode generation). We created a CanonicalizeHandleScope across parse/compile however this impacts performance (~5-8% on CodeLoad). Now that AST internalization is no longer done on-the-fly, we can localize the CanonicalHandleScope to only the internalization and renumbering phases where objects are created which could end up in the constant array pool. This seems to address the performance regression. BUG=v8:5203,chromium:634953 ========== to ========== [Interpreter] Localize the CanonicalHandleScope to parsing and renumbering. Ignition requires that objects which will be inserted into the constant pool are canonicalized (to enable off-thread bytecode generation). We created a CanonicalizeHandleScope across parse/compile however this impacts performance (~5-8% on CodeLoad). Now we localize the CanonicalHandleScope to only the parse/internalization and renumbering phases where objects are created which could end up in the constant array pool. This seems to address the performance regression. BUG=v8:5203,chromium:634953 ==========
rmcilroy@chromium.org changed reviewers: + mstarzinger@chromium.org
Updated to move the CanonicalHandleScope out of ast-value-factory.cc. This moves it mostly back to compiler.cc, although we still need the scopes in api.cc and compiler-dispatcher-job.cc. PTAL, does address your concerns? Michi: PTAL at compiler.cc changes.
The CQ bit was checked by rmcilroy@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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Interpreter] Localize the CanonicalHandleScope to parsing and renumbering. Ignition requires that objects which will be inserted into the constant pool are canonicalized (to enable off-thread bytecode generation). We created a CanonicalizeHandleScope across parse/compile however this impacts performance (~5-8% on CodeLoad). Now we localize the CanonicalHandleScope to only the parse/internalization and renumbering phases where objects are created which could end up in the constant array pool. This seems to address the performance regression. BUG=v8:5203,chromium:634953 ========== to ========== [Interpreter] Localize the CanonicalHandleScope to parsing and renumbering. Ignition requires that objects which will be inserted into the constant pool are canonicalized (to enable off-thread bytecode generation). We created a CanonicalizeHandleScope across parse/compile however this impacts performance (~5-8% on CodeLoad). Now we localize the CanonicalHandleScope to only the parse / internalization and renumbering phases where objects are created which could end up in the constant array pool. This seems to address the performance regression. BUG=v8:5203,chromium:634953 ==========
Even more lgtm now :)
LGTM.
The CQ bit was checked by rmcilroy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, mstarzinger@chromium.org, marja@chromium.org Link to the patchset: https://codereview.chromium.org/2318653002/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Localize the CanonicalHandleScope to parsing and renumbering. Ignition requires that objects which will be inserted into the constant pool are canonicalized (to enable off-thread bytecode generation). We created a CanonicalizeHandleScope across parse/compile however this impacts performance (~5-8% on CodeLoad). Now we localize the CanonicalHandleScope to only the parse / internalization and renumbering phases where objects are created which could end up in the constant array pool. This seems to address the performance regression. BUG=v8:5203,chromium:634953 ========== to ========== [Interpreter] Localize the CanonicalHandleScope to parsing and renumbering. Ignition requires that objects which will be inserted into the constant pool are canonicalized (to enable off-thread bytecode generation). We created a CanonicalizeHandleScope across parse/compile however this impacts performance (~5-8% on CodeLoad). Now we localize the CanonicalHandleScope to only the parse / internalization and renumbering phases where objects are created which could end up in the constant array pool. This seems to address the performance regression. BUG=v8:5203,chromium:634953 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Localize the CanonicalHandleScope to parsing and renumbering. Ignition requires that objects which will be inserted into the constant pool are canonicalized (to enable off-thread bytecode generation). We created a CanonicalizeHandleScope across parse/compile however this impacts performance (~5-8% on CodeLoad). Now we localize the CanonicalHandleScope to only the parse / internalization and renumbering phases where objects are created which could end up in the constant array pool. This seems to address the performance regression. BUG=v8:5203,chromium:634953 ========== to ========== [Interpreter] Localize the CanonicalHandleScope to parsing and renumbering. Ignition requires that objects which will be inserted into the constant pool are canonicalized (to enable off-thread bytecode generation). We created a CanonicalizeHandleScope across parse/compile however this impacts performance (~5-8% on CodeLoad). Now we localize the CanonicalHandleScope to only the parse / internalization and renumbering phases where objects are created which could end up in the constant array pool. This seems to address the performance regression. BUG=v8:5203,chromium:634953 Committed: https://crrev.com/b37daacd6b94086ad63ab763587ab2a7b8c88c6b Cr-Commit-Position: refs/heads/master@{#39443} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b37daacd6b94086ad63ab763587ab2a7b8c88c6b Cr-Commit-Position: refs/heads/master@{#39443} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
