Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1834373003:
[heap] Add optimized RecordWrites (Closed)
|
Created:
4 years, 8 months ago by Camillo Bruni Modified:
4 years, 8 months ago CC:
Hannes Payer (out of office), ulan, 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[heap] Add optimized RecordWrites
BUG=
Committed: https://crrev.com/5210f167e802a3758aac1f2900a6560c8de07831
Cr-Commit-Position: refs/heads/master@{#35231}
Committed: https://crrev.com/7b2861e35c9773273551f9d0335c090570d059fa
Cr-Commit-Position: refs/heads/master@{#35516}
Patch Set 1 #Patch Set 2 : adding helper macro #
Total comments: 12
Patch Set 3 : adressing comments #
Total comments: 2
Patch Set 4 : addressing comments #
Messages
Total messages: 34 (12 generated)
cbruni@chromium.org changed reviewers: + ulan@chromium.org
PTAL
PTAL again, the macro name is not ideal...
ulan@chromium.org changed reviewers: + hpayer@chromium.org
https://codereview.chromium.org/1834373003/diff/20001/src/heap/heap-inl.h File src/heap/heap-inl.h (right): https://codereview.chromium.org/1834373003/diff/20001/src/heap/heap-inl.h#new... src/heap/heap-inl.h:410: if (!InNewSpace((array->data_start() + offset)[i])) continue; why not simply array->get(offset + i)? https://codereview.chromium.org/1834373003/diff/20001/src/heap/heap-inl.h#new... src/heap/heap-inl.h:412: Page::FromAddress(reinterpret_cast<Address>(array)), Let's save the page in local variable before the loop starts. https://codereview.chromium.org/1834373003/diff/20001/src/heap/heap-inl.h#new... src/heap/heap-inl.h:413: HeapObject::cast(array)->address() + Let's use array->RawFieldOfElementAt(i) https://codereview.chromium.org/1834373003/diff/20001/src/objects-inl.h File src/objects-inl.h (right): https://codereview.chromium.org/1834373003/diff/20001/src/objects-inl.h#newco... src/objects-inl.h:1152: heap->RecordWrites(array, start, length); \ Let's put the two statements in do { } while (false) so that they do not get separated at macro usage site.
https://codereview.chromium.org/1834373003/diff/20001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/1834373003/diff/20001/src/elements.cc#newcode... src/elements.cc:1014: // HeapNumbers are, hence we have to write them again. Update this comment. https://codereview.chromium.org/1834373003/diff/20001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1834373003/diff/20001/src/heap/heap.h#newcode... src/heap/heap.h:1081: inline void RecordWrites(FixedArray* array, int offset, int length); Let's call it RecordFixedArrayElements. https://codereview.chromium.org/1834373003/diff/20001/src/objects-inl.h File src/objects-inl.h (right): https://codereview.chromium.org/1834373003/diff/20001/src/objects-inl.h#newco... src/objects-inl.h:1151: #define FIXED_ARRAY_WRITE_BARRIER(heap, array, start, length) \ FIXED_ARRAY_ELEMENTS_WRITE_BARRIER
ulan@chromium.org changed reviewers: + mlippautz@chromium.org
PTAL again. https://codereview.chromium.org/1834373003/diff/20001/src/heap/heap-inl.h File src/heap/heap-inl.h (right): https://codereview.chromium.org/1834373003/diff/20001/src/heap/heap-inl.h#new... src/heap/heap-inl.h:410: if (!InNewSpace((array->data_start() + offset)[i])) continue; On 2016/04/01 at 11:28:32, ulan wrote: > why not simply array->get(offset + i)? indeed... https://codereview.chromium.org/1834373003/diff/20001/src/heap/heap-inl.h#new... src/heap/heap-inl.h:412: Page::FromAddress(reinterpret_cast<Address>(array)), On 2016/04/01 at 11:28:32, ulan wrote: > Let's save the page in local variable before the loop starts. right... I remember that I measure some 10% difference when moving the page out of the loop (some weird clang optimization side-effect), probably just some flake, can't reproduce. https://codereview.chromium.org/1834373003/diff/20001/src/heap/heap-inl.h#new... src/heap/heap-inl.h:413: HeapObject::cast(array)->address() + On 2016/04/01 at 11:28:32, ulan wrote: > Let's use array->RawFieldOfElementAt(i) done https://codereview.chromium.org/1834373003/diff/20001/src/objects-inl.h File src/objects-inl.h (right): https://codereview.chromium.org/1834373003/diff/20001/src/objects-inl.h#newco... src/objects-inl.h:1151: #define FIXED_ARRAY_WRITE_BARRIER(heap, array, start, length) \ On 2016/04/01 at 11:34:40, Hannes Payer wrote: > FIXED_ARRAY_ELEMENTS_WRITE_BARRIER done. https://codereview.chromium.org/1834373003/diff/20001/src/objects-inl.h#newco... src/objects-inl.h:1152: heap->RecordWrites(array, start, length); \ On 2016/04/01 at 11:28:32, ulan wrote: > Let's put the two statements in > > do { > > } while (false) > > so that they do not get separated at macro usage site. done.
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/1834373003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1834373003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. FYI: I am going to refactor the heap interface for barriers after you land it. Specifically, I plan to make Heap::RecordWrite and Heap::RecordWrites to take care of incremental marking barrier so that incremental_marking is not exposed outside the heap. I will also make Heap::RecordWrites generic for a block of memory (not specific to FixedArray).
On 2016/04/04 at 09:26:54, ulan wrote: > LGTM. FYI: I am going to refactor the heap interface for barriers after you land it. Specifically, I plan to make Heap::RecordWrite and Heap::RecordWrites to take care of incremental marking barrier so that incremental_marking is not exposed outside the heap. I will also make Heap::RecordWrites generic for a block of memory (not specific to FixedArray). cool, sounds good :)
The CQ bit was checked by cbruni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1834373003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1834373003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [heap] Add optimized RecordWrites BUG= ========== to ========== [heap] Add optimized RecordWrites BUG= Committed: https://crrev.com/5210f167e802a3758aac1f2900a6560c8de07831 Cr-Commit-Position: refs/heads/master@{#35231} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5210f167e802a3758aac1f2900a6560c8de07831 Cr-Commit-Position: refs/heads/master@{#35231}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1860443003/ by machenbach@chromium.org. The reason for reverting is: [Sheriff] Likely causing blink crashes: https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064%20%....
PTAL again, this got reverted a while back, didn't notice until today
On 2016/04/08 at 15:05:29, cbruni wrote: > PTAL again, this got reverted a while back, didn't notice until today dbc: The failing blink tests hint at a missed WB, as we find an object in the FROM part of new space, meaning that we are following an outdated pointer.
Camillo, I think I found bugs that were causing the crashes. Sorry for missing them in the initial review. Could you please upload a separate reland CL? https://codereview.chromium.org/1834373003/diff/40001/src/heap/heap-inl.h File src/heap/heap-inl.h (right): https://codereview.chromium.org/1834373003/diff/40001/src/heap/heap-inl.h#new... src/heap/heap-inl.h:411: if (!InNewSpace(array->get(i))) continue; This should be if (!InNewSpace(array->get(offset + i))) https://codereview.chromium.org/1834373003/diff/40001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1834373003/diff/40001/src/heap/heap.cc#newcod... src/heap/heap.cc:1113: FIXED_ARRAY_ELEMENTS_WRITE_BARRIER(this, array, dst_index, src_index); src_index should be len.
On 2016/04/11 at 12:49:23, ulan wrote: > Camillo, I think I found bugs that were causing the crashes. > > Sorry for missing them in the initial review. > > Could you please upload a separate reland CL? > > https://codereview.chromium.org/1834373003/diff/40001/src/heap/heap-inl.h > File src/heap/heap-inl.h (right): > > https://codereview.chromium.org/1834373003/diff/40001/src/heap/heap-inl.h#new... > src/heap/heap-inl.h:411: if (!InNewSpace(array->get(i))) continue; > This should be if (!InNewSpace(array->get(offset + i))) > > https://codereview.chromium.org/1834373003/diff/40001/src/heap/heap.cc > File src/heap/heap.cc (right): > > https://codereview.chromium.org/1834373003/diff/40001/src/heap/heap.cc#newcod... > src/heap/heap.cc:1113: FIXED_ARRAY_ELEMENTS_WRITE_BARRIER(this, array, dst_index, src_index); > src_index should be len. awesome ulan! didn't have time to look at it in more detail.
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/1834373003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1834373003/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 cbruni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org Link to the patchset: https://codereview.chromium.org/1834373003/#ps60001 (title: "addressing comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1834373003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1834373003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [heap] Add optimized RecordWrites BUG= Committed: https://crrev.com/5210f167e802a3758aac1f2900a6560c8de07831 Cr-Commit-Position: refs/heads/master@{#35231} ========== to ========== [heap] Add optimized RecordWrites BUG= Committed: https://crrev.com/5210f167e802a3758aac1f2900a6560c8de07831 Cr-Commit-Position: refs/heads/master@{#35231} Committed: https://crrev.com/7b2861e35c9773273551f9d0335c090570d059fa Cr-Commit-Position: refs/heads/master@{#35516} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7b2861e35c9773273551f9d0335c090570d059fa Cr-Commit-Position: refs/heads/master@{#35516} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
