Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1812753004:
[elements] Minor hardening and cleanup of concat (Closed)
|
Created:
4 years, 9 months ago by Camillo Bruni Modified:
4 years, 9 months ago Reviewers:
Jakob Kummerow CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[elements] Minor hardening and cleanup of concat
BUG=
Committed: https://crrev.com/b98b3fbbe3dd14548cb356339f52403c07ef33f4
Cr-Commit-Position: refs/heads/master@{#35027}
Patch Set 1 #
Total comments: 2
Patch Set 2 : reverting part of the changes and using better var names #Patch Set 3 : moving var #
Total comments: 2
Patch Set 4 : initializing bools #Patch Set 5 : fixing var initialization #Patch Set 6 : another missing var init #Patch Set 7 : fixing wrong dcheck #Messages
Total messages: 27 (12 generated)
cbruni@chromium.org changed reviewers: + jkummerow@chromium.org
PTAL
https://codereview.chromium.org/1812753004/diff/1/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/1812753004/diff/1/src/elements.cc#newcode2887 src/elements.cc:2887: ArrayStorageAllocationMode mode = IsFastDoubleElementsKind(elements_kind) This is not the same. Consider the scenario mentioned in the comment right above: a large double array and a small array with object elements are concatenated. |elements_kind| at this point will be FAST_ELEMENTS (or holey), but the new backing store must be initialized with hole values, because one of the heap number allocations could cause the GC to scan the partially populated new backing store. You may want to add a test case for this :-) (Come to think of it, I'm not sure if the above is still true with the recently introduced black allocation... Please talk to a GC expert to decide whether it's OK to rely on this.)
On 2016/03/18 10:36:39, Jakob wrote: > (Come to think of it, I'm not sure if the above is still true with the recently > introduced black allocation... Please talk to a GC expert to decide whether it's > OK to rely on this.) Update: I've talked to Hannes; he said we should not rely on this. I.e. we should continue to initialize the newly allocated backing store before potentially triggering GC.
PTAL again https://codereview.chromium.org/1812753004/diff/1/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/1812753004/diff/1/src/elements.cc#newcode2887 src/elements.cc:2887: ArrayStorageAllocationMode mode = IsFastDoubleElementsKind(elements_kind) On 2016/03/18 at 10:36:39, Jakob wrote: > This is not the same. Consider the scenario mentioned in the comment right above: a large double array and a small array with object elements are concatenated. |elements_kind| at this point will be FAST_ELEMENTS (or holey), but the new backing store must be initialized with hole values, because one of the heap number allocations could cause the GC to scan the partially populated new backing store. You may want to add a test case for this :-) > (Come to think of it, I'm not sure if the above is still true with the recently introduced black allocation... Please talk to a GC expert to decide whether it's OK to rely on this.) Wonder what I was doing... I actually got that part.. but now looking at the code it doesn't make sense...
LGTM if you initialize the bools. https://codereview.chromium.org/1812753004/diff/40001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/1812753004/diff/40001/src/elements.cc#newcode... src/elements.cc:2858: bool has_raw_doubles; initialize to 'false' to avoid random behavior! https://codereview.chromium.org/1812753004/diff/40001/src/elements.cc#newcode... src/elements.cc:2862: bool is_holey; same: initialize!
The CQ bit was checked by cbruni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/1812753004/#ps60001 (title: "initializing bools")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1812753004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1812753004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_win_nosnap_shared_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_compil...)
The CQ bit was checked by cbruni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/1812753004/#ps80001 (title: "fixing var initialization")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1812753004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1812753004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_win_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/4826)
The CQ bit was checked by cbruni@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/1812753004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1812753004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/4836)
The CQ bit was checked by cbruni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/1812753004/#ps120001 (title: "fixing wrong dcheck")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1812753004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1812753004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [elements] Minor hardening and cleanup of concat BUG= ========== to ========== [elements] Minor hardening and cleanup of concat BUG= Committed: https://crrev.com/b98b3fbbe3dd14548cb356339f52403c07ef33f4 Cr-Commit-Position: refs/heads/master@{#35027} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b98b3fbbe3dd14548cb356339f52403c07ef33f4 Cr-Commit-Position: refs/heads/master@{#35027}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/1825363002/ by machenbach@chromium.org. The reason for reverting is: [Sheriff] Something seems to leak: https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20ASAN/builds/10838 I don't see the direct connection to this CL though.... |
