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

Issue 2336643002: [Interpreter] Move context chain search loop to handler (Closed)

Created:
4 years, 3 months ago by Leszek Swirski
Modified:
4 years, 3 months ago
CC:
v8-reviews_googlegroups.com, rmcilroy
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[Interpreter] Move context chain search loop to handler Moves the context chain search loop out of generated bytecode, and into the (Lda|Ldr|Sda)ContextSlot handler, by passing the context depth in as an additional operand. This should decrease the bytecode size and increase performance for deep context chain searches, at the cost of slightly increasing bytecode size for shallow context access. Committed: https://crrev.com/1c0c5fda2699cba9eea89b6d250a3a52d1965198 Cr-Commit-Position: refs/heads/master@{#39378}

Patch Set 1 #

Total comments: 18

Patch Set 2 : Address comments #

Total comments: 12

Patch Set 3 : Fix documentation nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+604 lines, -587 lines) Patch
M src/compiler/bytecode-graph-builder.cc View 2 chunks +6 lines, -10 lines 0 comments Download
M src/interpreter/bytecode-array-builder.h View 1 2 1 chunk +9 lines, -5 lines 0 comments Download
M src/interpreter/bytecode-array-builder.cc View 1 chunk +6 lines, -4 lines 0 comments Download
M src/interpreter/bytecode-generator.cc View 1 9 chunks +19 lines, -45 lines 0 comments Download
M src/interpreter/bytecodes.h View 1 chunk +3 lines, -3 lines 0 comments Download
M src/interpreter/interpreter.cc View 1 2 1 chunk +16 lines, -9 lines 0 comments Download
M src/interpreter/interpreter-assembler.h View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M src/interpreter/interpreter-assembler.cc View 1 2 1 chunk +30 lines, -0 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/BasicLoops.golden View 2 chunks +10 lines, -10 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/BreakableBlocks.golden View 2 chunks +14 lines, -14 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/CallLookupSlot.golden View 1 chunk +4 lines, -4 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/ClassDeclarations.golden View 6 chunks +7 lines, -7 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/CompoundExpressions.golden View 1 chunk +4 lines, -4 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/ConstVariableContextSlot.golden View 4 chunks +15 lines, -15 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/ContextParameters.golden View 4 chunks +10 lines, -10 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/ContextVariables.golden View 6 chunks +268 lines, -268 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/CountOperators.golden View 2 chunks +8 lines, -8 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/CreateArguments.golden View 2 chunks +6 lines, -6 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/Delete.golden View 1 chunk +3 lines, -3 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/Eval.golden View 1 chunk +4 lines, -4 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/ForOf.golden View 16 chunks +22 lines, -22 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/Generators.golden View 25 chunks +84 lines, -83 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/GlobalDelete.golden View 2 chunks +6 lines, -6 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/LetVariableContextSlot.golden View 4 chunks +17 lines, -17 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/LookupSlot.golden View 3 chunks +12 lines, -12 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/OuterContextVariables.golden View 2 chunks +9 lines, -12 lines 0 comments Download
M test/unittests/interpreter/bytecode-array-builder-unittest.cc View 3 chunks +4 lines, -4 lines 0 comments Download
M test/unittests/interpreter/bytecode-peephole-optimizer-unittest.cc View 1 chunk +4 lines, -2 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 15 (4 generated)
Leszek
mythria@chromium.org: Please review changes in rmcilroy@chromium.org: Please review changes in
4 years, 3 months ago (2016-09-12 16:16:02 UTC) #2
rmcilroy
Looks great. Thanks for the fast CL. Just some nits. https://codereview.chromium.org/2336643002/diff/1/src/interpreter/bytecode-generator.cc File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2336643002/diff/1/src/interpreter/bytecode-generator.cc#newcode926 ...
4 years, 3 months ago (2016-09-12 17:07:15 UTC) #3
Leszek Swirski
https://codereview.chromium.org/2336643002/diff/1/src/interpreter/bytecode-generator.cc File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2336643002/diff/1/src/interpreter/bytecode-generator.cc#newcode926 src/interpreter/bytecode-generator.cc:926: builder()->LoadTheHole().StoreContextSlot(execution_context()->reg(), On 2016/09/12 17:07:15, rmcilroy wrote: > Could you ...
4 years, 3 months ago (2016-09-13 09:42:35 UTC) #4
Michael Starzinger
Looking good. Just nits. https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/bytecode-array-builder.h File src/interpreter/bytecode-array-builder.h (right): https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/bytecode-array-builder.h#newcode97 src/interpreter/bytecode-array-builder.h:97: // Load the object at ...
4 years, 3 months ago (2016-09-13 09:53:08 UTC) #6
Leszek Swirski
https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/bytecode-array-builder.h File src/interpreter/bytecode-array-builder.h (right): https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/bytecode-array-builder.h#newcode97 src/interpreter/bytecode-array-builder.h:97: // Load the object at |slot_index| at |depth| in ...
4 years, 3 months ago (2016-09-13 10:17:51 UTC) #7
mythria
Nice, lgtm.
4 years, 3 months ago (2016-09-13 10:21:21 UTC) #8
rmcilroy
lgtm, thanks
4 years, 3 months ago (2016-09-13 10:32:23 UTC) #9
Michael Starzinger
LGTM. Thanks!
4 years, 3 months ago (2016-09-13 10:35:49 UTC) #10
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/2336643002/40001
4 years, 3 months ago (2016-09-13 10:39:40 UTC) #12
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 3 months ago (2016-09-13 11:09:07 UTC) #13
commit-bot: I haz the power
4 years, 3 months ago (2016-09-13 11:09:44 UTC) #15
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/1c0c5fda2699cba9eea89b6d250a3a52d1965198
Cr-Commit-Position: refs/heads/master@{#39378}

Powered by Google App Engine
This is Rietveld 408576698