Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1588903002:
[Interpreter] Fixes VisitObjectLiteral to reserve consecutive registers in innerscope. (Closed)
Description[Interpreter] Fixes VisitObjectLiteral to reserve consecutive registers in innerscope.
VisitObjectLiteral has two parts. First it creates a literal and then
sets properties or accessor properties. Setting properties requires a
runtime call and it expects the literal object which was created in the
first part is contiguous with other registers it allocates. Since these
are allocated in a different scope they are not always contiguous.
This causes problems with mjsunit/setter-on-constructor-prototype.js.
This cl fixes by allocating contiguous registers in the inner scope.
Literal value is copied into the newly allocated register so that all
the required registers are always contiguous.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/8ae9fb69c2363a40917364c0a22b7c09cb6dbc3c
Cr-Commit-Position: refs/heads/master@{#33371}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Changes it to allocate contiguous registers in the inner scope. #Patch Set 3 : rebased the patch. #
Total comments: 12
Patch Set 4 : Fixed comments from Ross. #Patch Set 5 : Rebased the patch. #
Messages
Total messages: 36 (18 generated)
mythria@chromium.org changed reviewers: + rmcilroy@chromium.org
PTAL, Thanks, Mythri https://codereview.chromium.org/1588903002/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1588903002/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:950: temporary_register_scope.PrepareForConsecutiveAllocations(5); This increases the number of registers used. So alternately, we could also move literal to a different register in the inner scope. I am not sure which is better.
https://codereview.chromium.org/1588903002/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1588903002/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:932: Register literal; As discussed offline, let's just always put literal into a register and move it to a temporary consecutive register in the inner scopes. https://codereview.chromium.org/1588903002/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:985: // key, value, language are in another. Remove this TODO
Description was changed from ========== [Interpreter] Fixes VisitObjectLiteral to reserve consecutive registers in outerscope. VisitObjectLiteral has two parts. First it creates a literal and then sets properties or accessor properties. Setting properties requires a runtime call and it expects the literal object which was created in the first part is contiguous with other registers it allocates. Since these are allocated in a different scope they are not always contiguous. This causes problems with mjsunit/setter-on-constructor-prototype.js. This cl fixes by allocating contiguous registers in the outer scope. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Fixes VisitObjectLiteral to reserve consecutive registers in innerscope. VisitObjectLiteral has two parts. First it creates a literal and then sets properties or accessor properties. Setting properties requires a runtime call and it expects the literal object which was created in the first part is contiguous with other registers it allocates. Since these are allocated in a different scope they are not always contiguous. This causes problems with mjsunit/setter-on-constructor-prototype.js. This cl fixes by allocating contiguous registers in the inner scope. Literal value is copied into the newly allocated register so that all the required registers are always contiguous. BUG=v8:4280 LOG=N ==========
PTAL, Thanks, Mythri https://codereview.chromium.org/1588903002/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1588903002/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:932: Register literal; On 2016/01/14 14:04:23, rmcilroy wrote: > As discussed offline, let's just always put literal into a register and move it > to a temporary consecutive register in the inner scopes. Done. https://codereview.chromium.org/1588903002/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:985: // key, value, language are in another. On 2016/01/14 14:04:23, rmcilroy wrote: > Remove this TODO Done.
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1588903002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1588903002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was unchecked by mythria@chromium.org
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/patch-status/1588903002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1588903002/40001
LGTM, thanks. https://codereview.chromium.org/1588903002/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1588903002/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:968: // Allocate in the outer scope since this register is returned. /s/since.../since this register is used to return the expression's results to the caller. https://codereview.chromium.org/1588903002/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:1008: Register::AreContiguous(literal_argument, key, value, language)); This DCHECK's probably not necessary anymore. https://codereview.chromium.org/1588903002/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:1031: DCHECK(Register::AreContiguous(literal_argument, value)); ditto https://codereview.chromium.org/1588903002/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:1064: Register::AreContiguous(literal_argument, name, getter, setter, attr)); ditto https://codereview.chromium.org/1588903002/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:1097: DCHECK(Register::AreContiguous(literal_argument, value)); ditto https://codereview.chromium.org/1588903002/diff/40001/test/mjsunit/mjsunit.st... File test/mjsunit/mjsunit.status (left): https://codereview.chromium.org/1588903002/diff/40001/test/mjsunit/mjsunit.st... test/mjsunit/mjsunit.status:1050: 'setter-on-constructor-prototype': [SKIP], Is this the only test which this fixes?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the review. Fixed all comments. https://codereview.chromium.org/1588903002/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1588903002/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:968: // Allocate in the outer scope since this register is returned. On 2016/01/15 17:15:09, rmcilroy wrote: > /s/since.../since this register is used to return the expression's results to > the caller. Thanks, Done. https://codereview.chromium.org/1588903002/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:1008: Register::AreContiguous(literal_argument, key, value, language)); On 2016/01/15 17:15:09, rmcilroy wrote: > This DCHECK's probably not necessary anymore. Done. https://codereview.chromium.org/1588903002/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:1031: DCHECK(Register::AreContiguous(literal_argument, value)); On 2016/01/15 17:15:09, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/1588903002/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:1064: Register::AreContiguous(literal_argument, name, getter, setter, attr)); On 2016/01/15 17:15:09, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/1588903002/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:1097: DCHECK(Register::AreContiguous(literal_argument, value)); On 2016/01/15 17:15:09, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/1588903002/diff/40001/test/mjsunit/mjsunit.st... File test/mjsunit/mjsunit.status (left): https://codereview.chromium.org/1588903002/diff/40001/test/mjsunit/mjsunit.st... test/mjsunit/mjsunit.status:1050: 'setter-on-constructor-prototype': [SKIP], On 2016/01/15 17:15:09, rmcilroy wrote: > Is this the only test which this fixes? There are three which pass locally. I removed them and I will try on bots. Thanks.
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1588903002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1588903002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mythria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1588903002/#ps60001 (title: "Fixed comments from Ross.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1588903002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1588903002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/9870)
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1588903002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1588903002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mythria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1588903002/#ps80001 (title: "Rebased the patch.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1588903002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1588903002/80001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Fixes VisitObjectLiteral to reserve consecutive registers in innerscope. VisitObjectLiteral has two parts. First it creates a literal and then sets properties or accessor properties. Setting properties requires a runtime call and it expects the literal object which was created in the first part is contiguous with other registers it allocates. Since these are allocated in a different scope they are not always contiguous. This causes problems with mjsunit/setter-on-constructor-prototype.js. This cl fixes by allocating contiguous registers in the inner scope. Literal value is copied into the newly allocated register so that all the required registers are always contiguous. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Fixes VisitObjectLiteral to reserve consecutive registers in innerscope. VisitObjectLiteral has two parts. First it creates a literal and then sets properties or accessor properties. Setting properties requires a runtime call and it expects the literal object which was created in the first part is contiguous with other registers it allocates. Since these are allocated in a different scope they are not always contiguous. This causes problems with mjsunit/setter-on-constructor-prototype.js. This cl fixes by allocating contiguous registers in the inner scope. Literal value is copied into the newly allocated register so that all the required registers are always contiguous. BUG=v8:4280 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Fixes VisitObjectLiteral to reserve consecutive registers in innerscope. VisitObjectLiteral has two parts. First it creates a literal and then sets properties or accessor properties. Setting properties requires a runtime call and it expects the literal object which was created in the first part is contiguous with other registers it allocates. Since these are allocated in a different scope they are not always contiguous. This causes problems with mjsunit/setter-on-constructor-prototype.js. This cl fixes by allocating contiguous registers in the inner scope. Literal value is copied into the newly allocated register so that all the required registers are always contiguous. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Fixes VisitObjectLiteral to reserve consecutive registers in innerscope. VisitObjectLiteral has two parts. First it creates a literal and then sets properties or accessor properties. Setting properties requires a runtime call and it expects the literal object which was created in the first part is contiguous with other registers it allocates. Since these are allocated in a different scope they are not always contiguous. This causes problems with mjsunit/setter-on-constructor-prototype.js. This cl fixes by allocating contiguous registers in the inner scope. Literal value is copied into the newly allocated register so that all the required registers are always contiguous. BUG=v8:4280 LOG=N Committed: https://crrev.com/8ae9fb69c2363a40917364c0a22b7c09cb6dbc3c Cr-Commit-Position: refs/heads/master@{#33371} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8ae9fb69c2363a40917364c0a22b7c09cb6dbc3c Cr-Commit-Position: refs/heads/master@{#33371} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
