Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2220673002:
[stubs] Cleanup FixedArray copying builtin (Closed)
Description[stubs] Cleanup FixedArray copying
Use common CodeStubAssembler routines for FixedArray-copying builtin.
Also cleanup a few shared pieces of code along the way.
BUG=chromium:608675
Committed: https://crrev.com/87448cdd0124b500351c79bb875e7d2ade9065c1
Cr-Commit-Position: refs/heads/master@{#38478}
Patch Set 1 #Patch Set 2 : Merge with latest #
Total comments: 2
Patch Set 3 : Rebase #Patch Set 4 : Tests pass #
Messages
Total messages: 34 (23 generated)
Description was changed from ========== [stubs] Cleanup FixedArray copying Use common CodeStubAssembler routines for FixedArray-copying builtin. BUG=chromium:608675 ========== to ========== [stubs] Cleanup FixedArray copying Use common CodeStubAssembler routines for FixedArray-copying builtin. Also cleanup a few shared pieces of code along the way. BUG=chromium:608675 ==========
danno@chromium.org changed reviewers: + bmeurer@chromium.org, mstarzinger@chromium.org
ptal
LGTM, thanks.
The CQ bit was checked by bmeurer@chromium.org
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
Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) 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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/10469) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10457) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/10414) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/6466) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/21175)
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.
We should figure out if it's OK to allocate large objects directly in old space. https://codereview.chromium.org/2220673002/diff/20001/src/builtins/builtins-i... File src/builtins/builtins-internal.cc (right): https://codereview.chromium.org/2220673002/diff/20001/src/builtins/builtins-i... src/builtins/builtins-internal.cc:88: Node* destination = assembler->AllocateFixedArray( Looking into this again, I don't think this is correct wrt. to what the GC expects, as we'd allocate FixedArrays that are larger than Page::kMaxRegularHeapObjectSize in old space here if they fit, although those should end up in large object space instead. Please check back with GC folks.
https://codereview.chromium.org/2220673002/diff/20001/src/builtins/builtins-i... File src/builtins/builtins-internal.cc (right): https://codereview.chromium.org/2220673002/diff/20001/src/builtins/builtins-i... src/builtins/builtins-internal.cc:88: Node* destination = assembler->AllocateFixedArray( On 2016/08/08 03:46:06, Benedikt Meurer wrote: > Looking into this again, I don't think this is correct wrt. to what the GC > expects, as we'd allocate FixedArrays that are larger than > Page::kMaxRegularHeapObjectSize in old space here if they fit, although those > should end up in large object space instead. Please check back with GC folks. Talked to the GC folks. Turns out that this will go down the slow path since it won't fit into an old-space page, and that will end up in the runtime. For better or worse, kPretenured is the way to specify old space for large allocations in the runtime call, the decision is made based on size inside the runtime routine. So if it doesn't fit in the page, it will be in old space... and the code here should be correct, albeit ugly.
The CQ bit was checked by danno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org Link to the patchset: https://codereview.chromium.org/2220673002/#ps20001 (title: "Merge with latest")
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
Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/6557)
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_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...)
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
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org Link to the patchset: https://codereview.chromium.org/2220673002/#ps60001 (title: "Tests pass")
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 ========== [stubs] Cleanup FixedArray copying Use common CodeStubAssembler routines for FixedArray-copying builtin. Also cleanup a few shared pieces of code along the way. BUG=chromium:608675 ========== to ========== [stubs] Cleanup FixedArray copying Use common CodeStubAssembler routines for FixedArray-copying builtin. Also cleanup a few shared pieces of code along the way. BUG=chromium:608675 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [stubs] Cleanup FixedArray copying Use common CodeStubAssembler routines for FixedArray-copying builtin. Also cleanup a few shared pieces of code along the way. BUG=chromium:608675 ========== to ========== [stubs] Cleanup FixedArray copying Use common CodeStubAssembler routines for FixedArray-copying builtin. Also cleanup a few shared pieces of code along the way. BUG=chromium:608675 Committed: https://crrev.com/87448cdd0124b500351c79bb875e7d2ade9065c1 Cr-Commit-Position: refs/heads/master@{#38478} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/87448cdd0124b500351c79bb875e7d2ade9065c1 Cr-Commit-Position: refs/heads/master@{#38478} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
