Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 4 months ago by ahaas Modified:
4 years, 4 months ago Reviewers:
Michael Lippautz, ulan CC:
Hannes Payer (out of office), Mircea Trofin, 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] Record references in the new code objects in heap::CopyCode.
R=mlippautz@chromium.org
BUG=chromium:633539
TEST=cctest/test-heap/TestNewSpaceRefsInCopiedCode
Committed: https://crrev.com/c088aea9222c312a78e9eae26fab7ffd57704c2e
Cr-Commit-Position: refs/heads/master@{#38326}
Patch Set 1 #
Total comments: 8
Patch Set 2 : unfriend the test. #Patch Set 3 : Remove DCHECK. #Patch Set 4 : Extract changes into RecordWritesIntoCode function. #Patch Set 5 : Init tmp pointer to avoid compiler warning #
Created: 4 years, 4 months ago
Messages
Total messages: 32 (24 generated)
The CQ bit was checked by ahaas@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_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/11673) v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
mlippautz@chromium.org changed reviewers: + ulan@chromium.org
+ulan https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.cc#newcode3377 src/heap/heap.cc:3377: for (RelocIterator it(new_code, That works, but ideally we would have fused visitor (together with the code in IterateBlackObject). In the case that we copy a black object you need to visit the reloc info twice, which is not ideal. Then again CopyCode might be rare enough so that it does not matter. Ulan, wdyt? https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.h#newcode1870 src/heap/heap.h:1870: friend AllocationResult TestCopyCode(Heap* heap, Code* code); No need to do that. Have a look at test/cctest/heap/heap-tester.h . Then just declare the test as HEAP_TEST(YourTest) {} and you have access to internals. https://codereview.chromium.org/2203783002/diff/1/test/cctest/heap/test-heap.cc File test/cctest/heap/test-heap.cc (right): https://codereview.chromium.org/2203783002/diff/1/test/cctest/heap/test-heap.... test/cctest/heap/test-heap.cc:126: Handle<Object> value = factory->NewNumber(1.000123); CHECK for object being in new space https://codereview.chromium.org/2203783002/diff/1/test/cctest/heap/test-heap.... test/cctest/heap/test-heap.cc:136: Handle<Code> code = isolate->factory()->NewCode( Cool stuff!
The CQ bit was checked by ahaas@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 ahaas@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...
https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.cc#newcode3377 src/heap/heap.cc:3377: for (RelocIterator it(new_code, On 2016/08/02 at 19:07:57, Michael Lippautz wrote: > That works, but ideally we would have fused visitor (together with the code in IterateBlackObject). In the case that we copy a black object you need to visit the reloc info twice, which is not ideal. Then again CopyCode might be rare enough so that it does not matter. > > Ulan, wdyt? I think that this would be an optimization that we should not do unless there are actual performance reasons for it. https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.h#newcode1870 src/heap/heap.h:1870: friend AllocationResult TestCopyCode(Heap* heap, Code* code); On 2016/08/02 at 19:07:57, Michael Lippautz wrote: > No need to do that. Have a look at > test/cctest/heap/heap-tester.h > . Then just declare the test as HEAP_TEST(YourTest) {} and you have access to internals. Done. https://codereview.chromium.org/2203783002/diff/1/test/cctest/heap/test-heap.cc File test/cctest/heap/test-heap.cc (right): https://codereview.chromium.org/2203783002/diff/1/test/cctest/heap/test-heap.... test/cctest/heap/test-heap.cc:126: Handle<Object> value = factory->NewNumber(1.000123); On 2016/08/02 at 19:07:58, Michael Lippautz wrote: > CHECK for object being in new space Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.cc#newcode3377 src/heap/heap.cc:3377: for (RelocIterator it(new_code, On 2016/08/03 08:47:42, ahaas wrote: > On 2016/08/02 at 19:07:57, Michael Lippautz wrote: > > That works, but ideally we would have fused visitor (together with the code in > IterateBlackObject). In the case that we copy a black object you need to visit > the reloc info twice, which is not ideal. Then again CopyCode might be rare > enough so that it does not matter. > > > > Ulan, wdyt? > > I think that this would be an optimization that we should not do unless there > are actual performance reasons for it. Yep, I would prefer decoding reloc info once. But for the sake of simplicity let's keep it as it is unless benchmarks regress. In any case, could you please extract new code into RecordWritesIntoCode function?
lgtm once Ulan's comment is addressed.
The CQ bit was checked by ahaas@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 ahaas@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 ahaas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlippautz@chromium.org, ulan@chromium.org Link to the patchset: https://codereview.chromium.org/2203783002/#ps80001 (title: "Init tmp pointer to avoid compiler warning")
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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [heap] Record references in the new code objects in heap::CopyCode. R=mlippautz@chromium.org BUG=chromium:633539 TEST=cctest/test-heap/TestNewSpaceRefsInCopiedCode ========== to ========== [heap] Record references in the new code objects in heap::CopyCode. R=mlippautz@chromium.org BUG=chromium:633539 TEST=cctest/test-heap/TestNewSpaceRefsInCopiedCode Committed: https://crrev.com/c088aea9222c312a78e9eae26fab7ffd57704c2e Cr-Commit-Position: refs/heads/master@{#38326} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c088aea9222c312a78e9eae26fab7ffd57704c2e Cr-Commit-Position: refs/heads/master@{#38326} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
