Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2796233003:
[heap] Evacuation for young generation (Closed)
|
Created:
3 years, 8 months ago by Michael Lippautz Modified:
3 years, 7 months ago CC:
v8-reviews_googlegroups.com, Hannes Payer (out of office), ulan, Michael Hablich Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
Description[heap] MinorMC: Evacuation for young generation
In the spirit of the full MC, we evacuate and update pointers in parallel for
the young generation.
The collectors are connected during incremental marking when mark bits are
transferred from the young generation bitmap to the old generation bitmap.
The evacuation phase cannot (yet) move pages and relies completely on copying
objects.
BUG=chromium:651354
Review-Url: https://codereview.chromium.org/2796233003
Cr-Commit-Position: refs/heads/master@{#45074}
Committed: https://chromium.googlesource.com/v8/v8/+/bf74d43de055393d97a3db4f01e4c832e35af10f
Patch Set 1 #Patch Set 2 : Fix global handles #Patch Set 3 : Fix external string table cleaning #Patch Set 4 : Fix page promotion tests #Patch Set 5 : Finish sweeping new space before minor mc #Patch Set 6 : More fixes #Patch Set 7 : Rebase #Patch Set 8 : Fix processing of black allocated objects #Patch Set 9 : Fix Expand #Patch Set 10 : Rebase #Patch Set 11 : Fix concurrent access on AccountingStats #Patch Set 12 : Add atomic version for TransferColor #Patch Set 13 : Rebase #Patch Set 14 : Rebase #Patch Set 15 : Smaller fixes #Patch Set 16 : Fix recording #
Total comments: 15
Patch Set 17 : Rebase after [heap] Allow concurrently transferring colors #
Total comments: 6
Patch Set 18 : Rebase after [heap] Refactor MC and introduce MarkCompactCollectorBase #Patch Set 19 : Rebase #Patch Set 20 : Rebase after smaller refactorings landed #
Total comments: 10
Patch Set 21 : Avoid keeping the list clustered into pages with and without free space #Patch Set 22 : Addressed comments #Patch Set 23 : Fix for accounting stats #Patch Set 24 : Fix accounting for committed/uncommitted memory #Patch Set 25 : Fixes after rebase #Patch Set 26 : Adjust RemovePageSafe heuristics #Patch Set 27 : Fix compaction space test #
Total comments: 8
Patch Set 28 : Addressed comments #Patch Set 29 : Fix limit #Patch Set 30 : Rebase after disabling black allocation during GCs #
Total comments: 10
Patch Set 31 : Addressed comments #Patch Set 32 : Fix some comments #Patch Set 33 : Disable flag #
Messages
Total messages: 151 (137 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [heap] Evacuaton for young generation BUG= ========== to ========== [heap] Evacuation for young generation BUG= ==========
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by mlippautz@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 checked by mlippautz@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...
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by mlippautz@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_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_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by mlippautz@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 mlippautz@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/25578) 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...)
The CQ bit was checked by mlippautz@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_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/23923) v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)
The CQ bit was checked by mlippautz@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_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/25733) v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)
The CQ bit was checked by mlippautz@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 ========== [heap] Evacuation for young generation BUG= ========== to ========== [heap] Evacuation for young generation BUG=chromium:651354 ==========
Description was changed from ========== [heap] Evacuation for young generation BUG=chromium:651354 ========== to ========== [heap] MinorMC: Evacuation for young generation In the spirit of the full MC, we evacuate and update pointers in parallel for the young generation. The collectors are connected during incremental marking when mark bits are transferred from the young generation bitmap to the old generation bitmap. BUG=chromium:651354 ==========
Description was changed from ========== [heap] MinorMC: Evacuation for young generation In the spirit of the full MC, we evacuate and update pointers in parallel for the young generation. The collectors are connected during incremental marking when mark bits are transferred from the young generation bitmap to the old generation bitmap. BUG=chromium:651354 ========== to ========== [heap] MinorMC: Evacuation for young generation In the spirit of the full MC, we evacuate and update pointers in parallel for the young generation. The collectors are connected during incremental marking when mark bits are transferred from the young generation bitmap to the old generation bitmap. BUG=chromium:651354 ==========
Description was changed from ========== [heap] MinorMC: Evacuation for young generation In the spirit of the full MC, we evacuate and update pointers in parallel for the young generation. The collectors are connected during incremental marking when mark bits are transferred from the young generation bitmap to the old generation bitmap. BUG=chromium:651354 ========== to ========== [heap] MinorMC: Evacuation for young generation In the spirit of the full MC, we evacuate and update pointers in parallel for the young generation. The collectors are connected during incremental marking when mark bits are transferred from the young generation bitmap to the old generation bitmap. The evacuation phase cannot (yet) move pages and relies completely on copying objects. BUG=chromium:651354 ==========
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 mlippautz@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 checked by mlippautz@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/25698) v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/25800)
The CQ bit was checked by mlippautz@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/25704) v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/25805)
The CQ bit was checked by mlippautz@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.
Patchset #13 (id:290001) has been deleted
The CQ bit was checked by mlippautz@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 mlippautz@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 mlippautz@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...) v8_linux64_verify_csa_rel_ng_triggered 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 mlippautz@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...
Patchset #17 (id:390001) has been deleted
Patchset #16 (id:370001) has been deleted
The CQ bit was checked by mlippautz@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.
mlippautz@chromium.org changed reviewers: + hpayer@chromium.org, ulan@chromium.org
Please take a look. The idea is to have the full CL here and land smaller changes until this gets manageable. https://codereview.chromium.org/2796233003/diff/410001/src/flag-definitions.h File src/flag-definitions.h (left): https://codereview.chromium.org/2796233003/diff/410001/src/flag-definitions.h... src/flag-definitions.h:677: DEFINE_NEG_IMPLICATION(minor_mc, page_promotion) page_promotion should still be enabled for full MC's, even though it's not supported by minor MC by now. https://codereview.chromium.org/2796233003/diff/410001/src/heap/incremental-m... File src/heap/incremental-marking.h (right): https://codereview.chromium.org/2796233003/diff/410001/src/heap/incremental-m... src/heap/incremental-marking.h:189: V8_INLINE static void TransferColor(HeapObject* from, HeapObject* to) { TransferColor might work concurrently in new space as multiple tasks allocate in a single page (LAB) for evacuation. https://codereview.chromium.org/2796233003/diff/410001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/2796233003/diff/410001/src/heap/mark-compact.... src/heap/mark-compact.cc:1947: RecordMigratedSlotVisitor::HostScope host_scope(record_visitor_, dst); This hack and all its consequences (stateful visitor) are only required until the host object is passed to the visitors. https://codereview.chromium.org/2796233003/diff/410001/src/heap/mark-compact.... src/heap/mark-compact.cc:1959: migration_observer_->Move(src, dst); I plan to make the use of MigrationObserver templatized, so that we only pay when we need it, which is in case of a minor MC during incremental marking. https://codereview.chromium.org/2796233003/diff/410001/src/heap/mark-compact.... src/heap/mark-compact.cc:3830: CreateAndExecuteEvacuationTasks<YoungGenerationEvacuator, The idea is to have a stateless YoungGenerationRecordMigratedSlotVisitor, so that we can also preallocate that one here and pass it in. This depends on Ulan's CL that threads through the host object into the visitor. https://codereview.chromium.org/2796233003/diff/410001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/2796233003/diff/410001/src/heap/spaces.h#newc... src/heap/spaces.h:1611: class AllocationStats BASE_EMBEDDED { We might borrow pages for evacuation (using the free lists) while at the same time expanding the space as the main thread also participates in evacuation. This is the reason these counters need to be atomic. https://codereview.chromium.org/2796233003/diff/410001/src/heap/spaces.h#newc... src/heap/spaces.h:1635: capacity_.Increment(bytes); This should be save even though there are 2 counters involved. We only ExpandSpace and ShrinkSpace non-concurrently.
I like it. https://codereview.chromium.org/2796233003/diff/430001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/2796233003/diff/430001/src/heap/mark-compact.... src/heap/mark-compact.h:425: class MinorMarkCompactCollector { Would it make sense to have a higher level abstract MarkCompact collector class from from which the two instances inherit and implement the interface? https://codereview.chromium.org/2796233003/diff/430001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/2796233003/diff/430001/src/heap/spaces.cc#new... src/heap/spaces.cc:2964: // only happen when we are evacuating for the young generation. do you mean: evacuating in minor mc? https://codereview.chromium.org/2796233003/diff/430001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/2796233003/diff/430001/src/heap/spaces.h#newc... src/heap/spaces.h:1611: AllocationStats() { Clear(); } Changing this to atomic values could be a separate CL.
https://codereview.chromium.org/2796233003/diff/410001/src/heap/incremental-m... File src/heap/incremental-marking.h (right): https://codereview.chromium.org/2796233003/diff/410001/src/heap/incremental-m... src/heap/incremental-marking.h:189: V8_INLINE static void TransferColor(HeapObject* from, HeapObject* to) { On 2017/04/21 07:05:51, Michael Lippautz wrote: > TransferColor might work concurrently in new space as multiple tasks allocate in > a single page (LAB) for evacuation. I guess you do not need to synchronize. https://codereview.chromium.org/2796233003/diff/410001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/2796233003/diff/410001/src/heap/mark-compact.... src/heap/mark-compact.cc:1947: RecordMigratedSlotVisitor::HostScope host_scope(record_visitor_, dst); On 2017/04/21 07:05:52, Michael Lippautz wrote: > This hack and all its consequences (stateful visitor) are only required until > the host object is passed to the visitors. Nice! https://codereview.chromium.org/2796233003/diff/410001/src/heap/mark-compact.... src/heap/mark-compact.cc:1959: migration_observer_->Move(src, dst); On 2017/04/21 07:05:52, Michael Lippautz wrote: > I plan to make the use of MigrationObserver templatized, so that we only pay > when we need it, which is in case of a minor MC during incremental marking. During evacuation you mean, right? https://codereview.chromium.org/2796233003/diff/410001/src/heap/mark-compact.... src/heap/mark-compact.cc:3830: CreateAndExecuteEvacuationTasks<YoungGenerationEvacuator, On 2017/04/21 07:05:52, Michael Lippautz wrote: > The idea is to have a stateless YoungGenerationRecordMigratedSlotVisitor, so > that we can also preallocate that one here and pass it in. > > This depends on Ulan's CL that threads through the host object into the visitor. Acknowledged. https://codereview.chromium.org/2796233003/diff/410001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/2796233003/diff/410001/src/heap/spaces.h#newc... src/heap/spaces.h:1611: class AllocationStats BASE_EMBEDDED { On 2017/04/21 07:05:52, Michael Lippautz wrote: > We might borrow pages for evacuation (using the free lists) while at the same > time expanding the space as the main thread also participates in evacuation. > This is the reason these counters need to be atomic. Acknowledged. https://codereview.chromium.org/2796233003/diff/410001/src/heap/spaces.h#newc... src/heap/spaces.h:1635: capacity_.Increment(bytes); On 2017/04/21 07:05:52, Michael Lippautz wrote: > This should be save even though there are 2 counters involved. We only > ExpandSpace and ShrinkSpace non-concurrently. Acknowledged.
https://codereview.chromium.org/2796233003/diff/410001/src/heap/incremental-m... File src/heap/incremental-marking.h (right): https://codereview.chromium.org/2796233003/diff/410001/src/heap/incremental-m... src/heap/incremental-marking.h:189: V8_INLINE static void TransferColor(HeapObject* from, HeapObject* to) { On 2017/04/21 14:46:26, Hannes Payer wrote: > On 2017/04/21 07:05:51, Michael Lippautz wrote: > > TransferColor might work concurrently in new space as multiple tasks allocate > in > > a single page (LAB) for evacuation. > > I guess you do not need to synchronize. We need to synchronize for live byte count which is hidden in ObjectMarking. E.g. multiple tasks might concurrently transfer colors onto the same page. https://codereview.chromium.org/2796233003/diff/410001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/2796233003/diff/410001/src/heap/mark-compact.... src/heap/mark-compact.cc:1959: migration_observer_->Move(src, dst); On 2017/04/21 14:46:26, Hannes Payer wrote: > On 2017/04/21 07:05:52, Michael Lippautz wrote: > > I plan to make the use of MigrationObserver templatized, so that we only pay > > when we need it, which is in case of a minor MC during incremental marking. > > During evacuation you mean, right? Yes. https://codereview.chromium.org/2796233003/diff/430001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/2796233003/diff/430001/src/heap/mark-compact.... src/heap/mark-compact.h:425: class MinorMarkCompactCollector { On 2017/04/21 14:35:18, Hannes Payer wrote: > Would it make sense to have a higher level abstract MarkCompact collector class > from from which the two instances inherit and implement the interface? Yes, I started implementing that in a smaller CL on the side. https://codereview.chromium.org/2796233003/diff/430001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/2796233003/diff/430001/src/heap/spaces.cc#new... src/heap/spaces.cc:2964: // only happen when we are evacuating for the young generation. On 2017/04/21 14:35:18, Hannes Payer wrote: > do you mean: evacuating in minor mc? Yes, will change. https://codereview.chromium.org/2796233003/diff/430001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/2796233003/diff/430001/src/heap/spaces.h#newc... src/heap/spaces.h:1611: AllocationStats() { Clear(); } On 2017/04/21 14:35:18, Hannes Payer wrote: > Changing this to atomic values could be a separate CL. Acknowledged.
Patchset #19 (id:470001) has been deleted
https://codereview.chromium.org/2796233003/diff/510001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/2796233003/diff/510001/src/heap/mark-compact.... src/heap/mark-compact.cc:309: VerifyEvacuation(heap_->new_space()); Verification should be done for the whole heap. https://codereview.chromium.org/2796233003/diff/510001/src/heap/mark-compact.... src/heap/mark-compact.cc:317: if (!heap_->InNewSpace(object)) return; Let's instead do a check that object is not in from space. https://codereview.chromium.org/2796233003/diff/510001/src/heap/mark-compact.... src/heap/mark-compact.cc:1667: MarkingState::External(heap_object))); Did you mean collector_.marking_state(heap_object) ? https://codereview.chromium.org/2796233003/diff/510001/src/heap/mark-compact.... src/heap/mark-compact.cc:1912: ObjectMarking::IsBlack(dst, state)) { if (ObjectMarking::IsBlack(dst, state)) { DCHECK(heap_->incremental_marking()->black_allocation()); ... } else { IncrementalMarking::TransferColor(); } https://codereview.chromium.org/2796233003/diff/510001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/2796233003/diff/510001/src/heap/spaces.h#newc... src/heap/spaces.h:527: bool CanUseForAllocation() { return CanAllocate() && !NeverEvacuate(); } Can you add a comment explaining how NeverEvacuate relates to allocation?
The CQ bit was checked by mlippautz@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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/25052) 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 mlippautz@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_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 mlippautz@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...
Patchset #23 (id:570001) has been deleted
Patchset #22 (id:550001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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_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...)
The CQ bit was checked by mlippautz@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...
Patchset #22 (id:590001) has been deleted
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 mlippautz@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_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_arm_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/26741) v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)
Patchset #22 (id:610001) has been deleted
The CQ bit was checked by mlippautz@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_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/...) v8_linux_verify_csa_rel_ng_triggered 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 mlippautz@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_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
The CQ bit was checked by mlippautz@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_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
All addressed. Also rebased after most recent CL that refactored out task creation. https://codereview.chromium.org/2796233003/diff/510001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/2796233003/diff/510001/src/heap/mark-compact.... src/heap/mark-compact.cc:309: VerifyEvacuation(heap_->new_space()); On 2017/04/26 09:51:28, ulan wrote: > Verification should be done for the whole heap. Done. https://codereview.chromium.org/2796233003/diff/510001/src/heap/mark-compact.... src/heap/mark-compact.cc:317: if (!heap_->InNewSpace(object)) return; On 2017/04/26 09:51:28, ulan wrote: > Let's instead do a check that object is not in from space. Done. https://codereview.chromium.org/2796233003/diff/510001/src/heap/mark-compact.... src/heap/mark-compact.cc:1667: MarkingState::External(heap_object))); On 2017/04/26 09:51:28, ulan wrote: > Did you mean collector_.marking_state(heap_object) ? Done. https://codereview.chromium.org/2796233003/diff/510001/src/heap/mark-compact.... src/heap/mark-compact.cc:1912: ObjectMarking::IsBlack(dst, state)) { On 2017/04/26 09:51:28, ulan wrote: > if (ObjectMarking::IsBlack(dst, state)) { > DCHECK(heap_->incremental_marking()->black_allocation()); > ... > } else { > IncrementalMarking::TransferColor(); > } Done. https://codereview.chromium.org/2796233003/diff/510001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/2796233003/diff/510001/src/heap/spaces.h#newc... src/heap/spaces.h:527: bool CanUseForAllocation() { return CanAllocate() && !NeverEvacuate(); } On 2017/04/26 09:51:28, ulan wrote: > Can you add a comment explaining how NeverEvacuate relates to allocation? This one is gone now since we take pages for which there exist free list entries. These are per definition pages that can be used for allocation.
The CQ bit was checked by mlippautz@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_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
The CQ bit was checked by mlippautz@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_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
The CQ bit was checked by mlippautz@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/26909) 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...)
https://codereview.chromium.org/2796233003/diff/730001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/2796233003/diff/730001/src/flag-definitions.h... src/flag-definitions.h:674: DEFINE_BOOL(minor_mc, true, "perform young generation mark compact GCs") Make sure you do not turn it on on ToT unless performance says "yey". https://codereview.chromium.org/2796233003/diff/730001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/2796233003/diff/730001/src/heap/mark-compact.... src/heap/mark-compact.cc:2727: // TODO(mlippautz): Instead of processing them explicitly, we should just add If you add them to the main marking deque while evacuating new space, you would not need to delay the processing and you would avoid the iteration overhead. https://codereview.chromium.org/2796233003/diff/730001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/2796233003/diff/730001/src/heap/mark-compact.... src/heap/mark-compact.h:491: void ProcessMarkingDeque(); ProcessMarkingDeque and EmtyMarkingDeque could also virtual. https://codereview.chromium.org/2796233003/diff/730001/src/heap/mark-compact.... src/heap/mark-compact.h:497: void EvacuateNewSpace(std::vector<HeapObject*>* black_allocation_objects); Evacuate* could be virtual if we do not expose black allocation and handle it within evacuation, c.f. comment in the implementation.
https://codereview.chromium.org/2796233003/diff/730001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/2796233003/diff/730001/src/flag-definitions.h... src/flag-definitions.h:674: DEFINE_BOOL(minor_mc, true, "perform young generation mark compact GCs") On 2017/05/02 16:17:26, Hannes Payer wrote: > Make sure you do not turn it on on ToT unless performance says "yey". Sure. https://codereview.chromium.org/2796233003/diff/730001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/2796233003/diff/730001/src/heap/mark-compact.... src/heap/mark-compact.cc:2727: // TODO(mlippautz): Instead of processing them explicitly, we should just add On 2017/05/02 16:17:26, Hannes Payer wrote: > If you add them to the main marking deque while evacuating new space, you would > not need to delay the processing and you would avoid the iteration overhead. The overhead is not noticeable. Also, it looks like it is a bad idea to introduce black-to-grey here. Lets discuss this offline. (It would work though because the concurrent marker is not running during the GC) https://codereview.chromium.org/2796233003/diff/730001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/2796233003/diff/730001/src/heap/mark-compact.... src/heap/mark-compact.h:491: void ProcessMarkingDeque(); On 2017/05/02 16:17:26, Hannes Payer wrote: > ProcessMarkingDeque and EmtyMarkingDeque could also virtual. Done. https://codereview.chromium.org/2796233003/diff/730001/src/heap/mark-compact.... src/heap/mark-compact.h:497: void EvacuateNewSpace(std::vector<HeapObject*>* black_allocation_objects); On 2017/05/02 16:17:26, Hannes Payer wrote: > Evacuate* could be virtual if we do not expose black allocation and handle it > within evacuation, c.f. comment in the implementation. See above.
The CQ bit was checked by mlippautz@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/26922) 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...)
The CQ bit was checked by mlippautz@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.
ptal
lgtm with few nits :) https://codereview.chromium.org/2796233003/diff/790001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/2796233003/diff/790001/src/heap/mark-compact.... src/heap/mark-compact.cc:325: if (heap()->InNewSpace(object)) { CHECK_IMPLIES would be more readable here. https://codereview.chromium.org/2796233003/diff/790001/src/heap/mark-compact.... src/heap/mark-compact.cc:1923: return ObjectMarking::IsBlack(object, collector_->marking_state(object)); How do we know that the collector_ is the full collector? Maybe pass in the heap and get heap->mark_compact_collector() explicitly? https://codereview.chromium.org/2796233003/diff/790001/src/heap/mark-compact.... src/heap/mark-compact.cc:2521: static bool IsUnmarkedObject(Heap* heap, Object** p) { Nit: change the name to mention young generation so that this function is not accidentally used by the full collector? Since the marking state refers to external bitmap.
LGTM https://codereview.chromium.org/2796233003/diff/790001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/2796233003/diff/790001/src/heap/mark-compact.... src/heap/mark-compact.cc:1883: heap_->incremental_marking()->TransferColor<MarkBit::ATOMIC>(src, dst); Note: Ideally, this one would be non-atomic. https://codereview.chromium.org/2796233003/diff/790001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/2796233003/diff/790001/src/heap/mark-compact.... src/heap/mark-compact.h:308: virtual void ProcessMarkingDeque() = 0; Can you move the comments from MarkCompacts to the abstract class and add specific details in comments to the implementations.
https://codereview.chromium.org/2796233003/diff/790001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/2796233003/diff/790001/src/heap/mark-compact.... src/heap/mark-compact.cc:325: if (heap()->InNewSpace(object)) { On 2017/05/03 14:27:20, ulan wrote: > CHECK_IMPLIES would be more readable here. Done. https://codereview.chromium.org/2796233003/diff/790001/src/heap/mark-compact.... src/heap/mark-compact.cc:1883: heap_->incremental_marking()->TransferColor<MarkBit::ATOMIC>(src, dst); On 2017/05/03 14:44:53, Hannes Payer wrote: > Note: Ideally, this one would be non-atomic. Acknowledged. https://codereview.chromium.org/2796233003/diff/790001/src/heap/mark-compact.... src/heap/mark-compact.cc:1923: return ObjectMarking::IsBlack(object, collector_->marking_state(object)); On 2017/05/03 14:27:20, ulan wrote: > How do we know that the collector_ is the full collector? Maybe pass in the heap > and get heap->mark_compact_collector() explicitly? collector_ is of type MarkCompactCollector, not the base type. https://codereview.chromium.org/2796233003/diff/790001/src/heap/mark-compact.... src/heap/mark-compact.cc:2521: static bool IsUnmarkedObject(Heap* heap, Object** p) { On 2017/05/03 14:27:20, ulan wrote: > Nit: change the name to mention young generation so that this function is not > accidentally used by the full collector? Since the marking state refers to > external bitmap. Done. https://codereview.chromium.org/2796233003/diff/790001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/2796233003/diff/790001/src/heap/mark-compact.... src/heap/mark-compact.h:308: virtual void ProcessMarkingDeque() = 0; On 2017/05/03 14:44:53, Hannes Payer wrote: > Can you move the comments from MarkCompacts to the abstract class and add > specific details in comments to the implementations. Done.
The CQ bit was checked by mlippautz@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 mlippautz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org, ulan@chromium.org Link to the patchset: https://codereview.chromium.org/2796233003/#ps850001 (title: "Disable flag")
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": 850001, "attempt_start_ts": 1493845286259270,
"parent_rev": "8ab39ebcf90e6e838dd5ca2d6bbfdd8111841777", "commit_rev":
"bf74d43de055393d97a3db4f01e4c832e35af10f"}
Message was sent while issue was closed.
Description was changed from ========== [heap] MinorMC: Evacuation for young generation In the spirit of the full MC, we evacuate and update pointers in parallel for the young generation. The collectors are connected during incremental marking when mark bits are transferred from the young generation bitmap to the old generation bitmap. The evacuation phase cannot (yet) move pages and relies completely on copying objects. BUG=chromium:651354 ========== to ========== [heap] MinorMC: Evacuation for young generation In the spirit of the full MC, we evacuate and update pointers in parallel for the young generation. The collectors are connected during incremental marking when mark bits are transferred from the young generation bitmap to the old generation bitmap. The evacuation phase cannot (yet) move pages and relies completely on copying objects. BUG=chromium:651354 Review-Url: https://codereview.chromium.org/2796233003 Cr-Commit-Position: refs/heads/master@{#45074} Committed: https://chromium.googlesource.com/v8/v8/+/bf74d43de055393d97a3db4f01e4c832e35... ==========
Message was sent while issue was closed.
Committed patchset #33 (id:850001) as https://chromium.googlesource.com/v8/v8/+/bf74d43de055393d97a3db4f01e4c832e35... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
