Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2829093004:
[turbofan] Avoid going through ArgumentsAdaptorTrampoline for CSA/C++ builtins (Closed)
Description[turbofan] Reland: Avoid going through ArgumentsAdaptorTrampoline for select CSA array builtins
This CL changes certain frequently-called Array builtins to use CodeStubArguments
rather than peek at the stack frames above array builtins to determine if options
arguments have been passed into them.
Previous failure likely due to unfortunate/unluckily timed GC that moved due to
changed timing/allocation from this CL. Test mitigation for allocation-site-info.js
included.
BUG=v8:1956
LOG=N
Review-Url: https://codereview.chromium.org/2829093004
Cr-Commit-Position: refs/heads/master@{#44998}
Committed: https://chromium.googlesource.com/v8/v8/+/455f9df04c138c88c52ede7d3f0e91ea087c4ee6
Patch Set 1 #Patch Set 2 : Remove stray changes #Patch Set 3 : Fix CSA verifier #Patch Set 4 : Merge with ToT #
Total comments: 14
Patch Set 5 : Review feedback #Patch Set 6 : Fix typo #
Total comments: 2
Patch Set 7 : Review feedback #Patch Set 8 : Fix typed array builtins #Patch Set 9 : Fix flakey test #Patch Set 10 : Add other files again #
Messages
Total messages: 58 (43 generated)
The CQ bit was checked by danno@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_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
Description was changed from ========== [turbofan] Avoid going through ArgumentsAdaptorTrampoline for CSA/C++ builtins This is a reland of #44593 with the necessary changes to the Array builtins to not have peek at the stack frames above array builtins to determine if options arguments have been passed to the builtins. Instead, the array builtins can now take argument argument counts and inspect the passed-in argc and use a CodeStubArguments object to access passed-in argument. ========== to ========== [turbofan] Avoid going through ArgumentsAdaptorTrampoline for CSA/C++ builtins This is a re-land of #44593 with the necessary changes to the Array builtins to not have peek at the stack frames above array builtins to determine if options arguments have been passed to the builtins. Instead, the array builtins can now take argument argument counts and inspect the passed-in argc and use a CodeStubArguments object to access passed-in argument. BUG=chromium: 709782 LOG=N ==========
Description was changed from ========== [turbofan] Avoid going through ArgumentsAdaptorTrampoline for CSA/C++ builtins This is a re-land of #44593 with the necessary changes to the Array builtins to not have peek at the stack frames above array builtins to determine if options arguments have been passed to the builtins. Instead, the array builtins can now take argument argument counts and inspect the passed-in argc and use a CodeStubArguments object to access passed-in argument. BUG=chromium: 709782 LOG=N ========== to ========== [turbofan] Avoid going through ArgumentsAdaptorTrampoline for CSA/C++ builtins This is a re-land of #44593 with the necessary changes to the Array builtins to not have peek at the stack frames above array builtins to determine if options arguments have been passed to the builtins. Instead, the array builtins can now take argument argument counts and inspect the passed-in argc and use a CodeStubArguments object to access passed-in argument. BUG=chromium:709782 LOG=N ==========
The CQ bit was checked by danno@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_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
The CQ bit was checked by danno@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: This issue passed the CQ dry run.
The CQ bit was checked by danno@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: This issue passed the CQ dry run.
danno@chromium.org changed reviewers: + bmeurer@chromium.org, ishell@chromium.org, mvstanton@chromium.org
PTAL, Igor please check the CSA changes.
Cool stuff. LGTM. I'm a little confused on the BUG= listed though...it's declared as fixed...is it rather that this is a different issue, somewhat orthogonal to the bug? (instead an optimization for something seen along the way?).
https://codereview.chromium.org/2829093004/diff/60001/src/compiler/js-call-re... File src/compiler/js-call-reducer.cc (right): https://codereview.chromium.org/2829093004/diff/60001/src/compiler/js-call-re... src/compiler/js-call-reducer.cc:585: // For CSA/C++ builtin {target}s, we can ensure that the number As discussed a few weeks ago, I don't think we should reland this. Instead we should change it on the builtins side and declare the interesting ones with kDontAdaptArgumentsSentinel. Then JSTypedLowering will generate direct calls for those.
https://codereview.chromium.org/2829093004/diff/60001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2829093004/diff/60001/src/bootstrapper.cc#new... src/bootstrapper.cc:4347: SharedFunctionInfo::kDontAdaptArgumentsSentinel); Idea for further improvement: it would be nice to get the argument_count value from the builtin's definition. Same applies to CreateFunction/SimpleCreateFunction/InstallFunction and friends. https://codereview.chromium.org/2829093004/diff/60001/src/builtins/builtins-a... File src/builtins/builtins-array-gen.cc (right): https://codereview.chromium.org/2829093004/diff/60001/src/builtins/builtins-a... src/builtins/builtins-array-gen.cc:249: PopAndReturn(IntPtrAdd(argc_, IntPtrConstant(1)), value); It would be nice to have a comment that +1 is here because argc does not include receiver. https://codereview.chromium.org/2829093004/diff/60001/src/builtins/builtins-a... src/builtins/builtins-array-gen.cc:1032: &ArrayBuiltinCodeStubAssembler::ReduceResultGenerator, TypedArrayPrototype* builtins are not adapted to the new ReduceResultGenerator yet. https://codereview.chromium.org/2829093004/diff/60001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2829093004/diff/60001/src/code-stub-assembler... src/code-stub-assembler.cc:8509: assembler_->GotoIf(assembler_->IntPtrOrSmiGreaterThanOrEqual( s/IntPtr/UintPtr/ to be in sync with AtIndex. https://codereview.chromium.org/2829093004/diff/60001/src/code-stub-assembler.h File src/code-stub-assembler.h (right): https://codereview.chromium.org/2829093004/diff/60001/src/code-stub-assembler... src/code-stub-assembler.h:1459: CodeStubArguments(CodeStubAssembler* assembler, Node* argc) While you are here please update the comment. It looks like this constructor expects |argc| as an intptr value and the second expects |argc| according to given |param_mode|. https://codereview.chromium.org/2829093004/diff/60001/src/interface-descripto... File src/interface-descriptors.h (right): https://codereview.chromium.org/2829093004/diff/60001/src/interface-descripto... src/interface-descriptors.h:658: DEFINE_PARAMETERS(kReceiver, kCallback, kThisArg, kArray, kObject, kInitialK, While you are here, please fix all the builtins that use this descriptor: they must be defined as TFC but not as TFJ. Otherwise we will have two inconsistent descriptor definitions. And you can also remove CodeFactory methods for these builtins in favor of Builtins::CallableFor.
The CQ bit was checked by danno@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 ========== [turbofan] Avoid going through ArgumentsAdaptorTrampoline for CSA/C++ builtins This is a re-land of #44593 with the necessary changes to the Array builtins to not have peek at the stack frames above array builtins to determine if options arguments have been passed to the builtins. Instead, the array builtins can now take argument argument counts and inspect the passed-in argc and use a CodeStubArguments object to access passed-in argument. BUG=chromium:709782 LOG=N ========== to ========== [turbofan] Avoid going through ArgumentsAdaptorTrampoline for select CSA/C++ builtins This CL changes certain frequently-called Array builtins to use CodeStubArguments rather than peek at the stack frames above array builtins to determine if options arguments have been passed into them. BUG=v8:1956 LOG=N ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
The CQ bit was checked by danno@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...
please take another look https://codereview.chromium.org/2829093004/diff/60001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2829093004/diff/60001/src/bootstrapper.cc#new... src/bootstrapper.cc:4347: SharedFunctionInfo::kDontAdaptArgumentsSentinel); On 2017/04/27 10:38:05, Igor Sheludko wrote: > Idea for further improvement: it would be nice to get the argument_count value > from the builtin's definition. Same applies to > CreateFunction/SimpleCreateFunction/InstallFunction and friends. As discussed, I'll do this in a later CL. https://codereview.chromium.org/2829093004/diff/60001/src/builtins/builtins-a... File src/builtins/builtins-array-gen.cc (right): https://codereview.chromium.org/2829093004/diff/60001/src/builtins/builtins-a... src/builtins/builtins-array-gen.cc:249: PopAndReturn(IntPtrAdd(argc_, IntPtrConstant(1)), value); On 2017/04/27 10:38:05, Igor Sheludko wrote: > It would be nice to have a comment that +1 is here because argc does not include > receiver. Done. https://codereview.chromium.org/2829093004/diff/60001/src/builtins/builtins-a... src/builtins/builtins-array-gen.cc:1032: &ArrayBuiltinCodeStubAssembler::ReduceResultGenerator, On 2017/04/27 10:38:05, Igor Sheludko wrote: > TypedArrayPrototype* builtins are not adapted to the new ReduceResultGenerator > yet. Done. https://codereview.chromium.org/2829093004/diff/60001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2829093004/diff/60001/src/code-stub-assembler... src/code-stub-assembler.cc:8509: assembler_->GotoIf(assembler_->IntPtrOrSmiGreaterThanOrEqual( On 2017/04/27 10:38:05, Igor Sheludko wrote: > s/IntPtr/UintPtr/ to be in sync with AtIndex. Done. https://codereview.chromium.org/2829093004/diff/60001/src/code-stub-assembler.h File src/code-stub-assembler.h (right): https://codereview.chromium.org/2829093004/diff/60001/src/code-stub-assembler... src/code-stub-assembler.h:1459: CodeStubArguments(CodeStubAssembler* assembler, Node* argc) On 2017/04/27 10:38:05, Igor Sheludko wrote: > While you are here please update the comment. It looks like this constructor > expects |argc| as an intptr value and the second expects |argc| according to > given |param_mode|. Done. https://codereview.chromium.org/2829093004/diff/60001/src/compiler/js-call-re... File src/compiler/js-call-reducer.cc (right): https://codereview.chromium.org/2829093004/diff/60001/src/compiler/js-call-re... src/compiler/js-call-reducer.cc:585: // For CSA/C++ builtin {target}s, we can ensure that the number On 2017/04/26 16:17:57, Benedikt Meurer wrote: > As discussed a few weeks ago, I don't think we should reland this. Instead we > should change it on the builtins side and declare the interesting ones with > kDontAdaptArgumentsSentinel. Then JSTypedLowering will generate direct calls for > those. Done. https://codereview.chromium.org/2829093004/diff/60001/src/interface-descripto... File src/interface-descriptors.h (right): https://codereview.chromium.org/2829093004/diff/60001/src/interface-descripto... src/interface-descriptors.h:658: DEFINE_PARAMETERS(kReceiver, kCallback, kThisArg, kArray, kObject, kInitialK, On 2017/04/27 10:38:05, Igor Sheludko wrote: > While you are here, please fix all the builtins that use this descriptor: they > must be defined as TFC but not as TFJ. Otherwise we will have two inconsistent > descriptor definitions. > And you can also remove CodeFactory methods for these builtins in favor of > Builtins::CallableFor. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm once the comment is addressed: https://codereview.chromium.org/2829093004/diff/100001/src/builtins/builtins-... File src/builtins/builtins-array-gen.cc (right): https://codereview.chromium.org/2829093004/diff/100001/src/builtins/builtins-... src/builtins/builtins-array-gen.cc:461: Return(a_.value()); ReturnFromBuiltin(a_.value()); Please try to run the tests with --experimental-fast-array-builtins.
Ran with --experimental-fast-array-builtins and fixed a remaining issue. Thanks for the review. https://codereview.chromium.org/2829093004/diff/100001/src/builtins/builtins-... File src/builtins/builtins-array-gen.cc (right): https://codereview.chromium.org/2829093004/diff/100001/src/builtins/builtins-... src/builtins/builtins-array-gen.cc:461: Return(a_.value()); On 2017/04/28 12:51:46, Igor Sheludko wrote: > ReturnFromBuiltin(a_.value()); > > Please try to run the tests with --experimental-fast-array-builtins. Good catch!
The CQ bit was checked by danno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvstanton@chromium.org, ishell@chromium.org Link to the patchset: https://codereview.chromium.org/2829093004/#ps140001 (title: "Fix typed array builtins")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493448859302770,
"parent_rev": "91d757bdd7a444017fea838f6fa94b98716dff3a", "commit_rev":
"680356278ddc7577e3b967fcc92055522ce00856"}
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Avoid going through ArgumentsAdaptorTrampoline for select CSA/C++ builtins This CL changes certain frequently-called Array builtins to use CodeStubArguments rather than peek at the stack frames above array builtins to determine if options arguments have been passed into them. BUG=v8:1956 LOG=N ========== to ========== [turbofan] Avoid going through ArgumentsAdaptorTrampoline for select CSA/C++ builtins This CL changes certain frequently-called Array builtins to use CodeStubArguments rather than peek at the stack frames above array builtins to determine if options arguments have been passed into them. BUG=v8:1956 LOG=N Review-Url: https://codereview.chromium.org/2829093004 Cr-Commit-Position: refs/heads/master@{#44994} Committed: https://chromium.googlesource.com/v8/v8/+/680356278ddc7577e3b967fcc92055522ce... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/v8/v8/+/680356278ddc7577e3b967fcc92055522ce...
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2851703005/ by danno@chromium.org. The reason for reverting is: Nosnap failure.
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Avoid going through ArgumentsAdaptorTrampoline for select CSA/C++ builtins This CL changes certain frequently-called Array builtins to use CodeStubArguments rather than peek at the stack frames above array builtins to determine if options arguments have been passed into them. BUG=v8:1956 LOG=N Review-Url: https://codereview.chromium.org/2829093004 Cr-Commit-Position: refs/heads/master@{#44994} Committed: https://chromium.googlesource.com/v8/v8/+/680356278ddc7577e3b967fcc92055522ce... ========== to ========== [turbofan] Reland: Avoid going through ArgumentsAdaptorTrampoline for select CSA array builtins This CL changes certain frequently-called Array builtins to use CodeStubArguments rather than peek at the stack frames above array builtins to determine if options arguments have been passed into them. Previous failure cannot be reproed with failing config. Flake? BUG=v8:1956 LOG=N Review-Url: https://codereview.chromium.org/2829093004 Cr-Commit-Position: refs/heads/master@{#44994} Committed: https://chromium.googlesource.com/v8/v8/+/680356278ddc7577e3b967fcc92055522ce... ==========
Description was changed from ========== [turbofan] Reland: Avoid going through ArgumentsAdaptorTrampoline for select CSA array builtins This CL changes certain frequently-called Array builtins to use CodeStubArguments rather than peek at the stack frames above array builtins to determine if options arguments have been passed into them. Previous failure cannot be reproed with failing config. Flake? BUG=v8:1956 LOG=N Review-Url: https://codereview.chromium.org/2829093004 Cr-Commit-Position: refs/heads/master@{#44994} Committed: https://chromium.googlesource.com/v8/v8/+/680356278ddc7577e3b967fcc92055522ce... ========== to ========== [turbofan] Reland: Avoid going through ArgumentsAdaptorTrampoline for select CSA array builtins This CL changes certain frequently-called Array builtins to use CodeStubArguments rather than peek at the stack frames above array builtins to determine if options arguments have been passed into them. Previous failure cannot be reproed with failing config. Flake? BUG=v8:1956 LOG=N ==========
The CQ bit was checked by danno@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493463133498880,
"parent_rev": "5896d38cfb9e3ae6be86b8aa22df258c78a7c758", "commit_rev":
"7ca381e84792b83581d0199dfae2888781785273"}
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Reland: Avoid going through ArgumentsAdaptorTrampoline for select CSA array builtins This CL changes certain frequently-called Array builtins to use CodeStubArguments rather than peek at the stack frames above array builtins to determine if options arguments have been passed into them. Previous failure cannot be reproed with failing config. Flake? BUG=v8:1956 LOG=N ========== to ========== [turbofan] Reland: Avoid going through ArgumentsAdaptorTrampoline for select CSA array builtins This CL changes certain frequently-called Array builtins to use CodeStubArguments rather than peek at the stack frames above array builtins to determine if options arguments have been passed into them. Previous failure cannot be reproed with failing config. Flake? BUG=v8:1956 LOG=N Review-Url: https://codereview.chromium.org/2829093004 Cr-Commit-Position: refs/heads/master@{#44996} Committed: https://chromium.googlesource.com/v8/v8/+/7ca381e84792b83581d0199dfae28887817... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/v8/v8/+/7ca381e84792b83581d0199dfae28887817...
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2851063002/ by danno@chromium.org. The reason for reverting is: Still fails. Likely has to do with gc heap size for allocation site tests, mitigation pending....
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Reland: Avoid going through ArgumentsAdaptorTrampoline for select CSA array builtins This CL changes certain frequently-called Array builtins to use CodeStubArguments rather than peek at the stack frames above array builtins to determine if options arguments have been passed into them. Previous failure cannot be reproed with failing config. Flake? BUG=v8:1956 LOG=N Review-Url: https://codereview.chromium.org/2829093004 Cr-Commit-Position: refs/heads/master@{#44996} Committed: https://chromium.googlesource.com/v8/v8/+/7ca381e84792b83581d0199dfae28887817... ========== to ========== [turbofan] Reland: Avoid going through ArgumentsAdaptorTrampoline for select CSA array builtins This CL changes certain frequently-called Array builtins to use CodeStubArguments rather than peek at the stack frames above array builtins to determine if options arguments have been passed into them. Previous failure cannot be reproed with failing config. Flake? BUG=v8:1956 LOG=N ==========
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Reland: Avoid going through ArgumentsAdaptorTrampoline for select CSA array builtins This CL changes certain frequently-called Array builtins to use CodeStubArguments rather than peek at the stack frames above array builtins to determine if options arguments have been passed into them. Previous failure cannot be reproed with failing config. Flake? BUG=v8:1956 LOG=N ========== to ========== [turbofan] Reland: Avoid going through ArgumentsAdaptorTrampoline for select CSA array builtins This CL changes certain frequently-called Array builtins to use CodeStubArguments rather than peek at the stack frames above array builtins to determine if options arguments have been passed into them. Previous failure likely due to unfortunate/unluckily timed GC that moved due to changed timing/allocation from this CL. Test mitigation for allocation-site-info.js included. BUG=v8:1956 LOG=N ==========
The CQ bit was checked by danno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org, mvstanton@chromium.org Link to the patchset: https://codereview.chromium.org/2829093004/#ps180001 (title: "Add other files again")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493464239539360,
"parent_rev": "6953bb4012bf3b3e9e56f1fb7ba89e0294d278c8", "commit_rev":
"455f9df04c138c88c52ede7d3f0e91ea087c4ee6"}
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Reland: Avoid going through ArgumentsAdaptorTrampoline for select CSA array builtins This CL changes certain frequently-called Array builtins to use CodeStubArguments rather than peek at the stack frames above array builtins to determine if options arguments have been passed into them. Previous failure likely due to unfortunate/unluckily timed GC that moved due to changed timing/allocation from this CL. Test mitigation for allocation-site-info.js included. BUG=v8:1956 LOG=N ========== to ========== [turbofan] Reland: Avoid going through ArgumentsAdaptorTrampoline for select CSA array builtins This CL changes certain frequently-called Array builtins to use CodeStubArguments rather than peek at the stack frames above array builtins to determine if options arguments have been passed into them. Previous failure likely due to unfortunate/unluckily timed GC that moved due to changed timing/allocation from this CL. Test mitigation for allocation-site-info.js included. BUG=v8:1956 LOG=N Review-Url: https://codereview.chromium.org/2829093004 Cr-Commit-Position: refs/heads/master@{#44998} Committed: https://chromium.googlesource.com/v8/v8/+/455f9df04c138c88c52ede7d3f0e91ea087... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/v8/v8/+/455f9df04c138c88c52ede7d3f0e91ea087... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
