Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1343333002:
[heap] Introduce parallel compaction algorithm. (Closed)
|
Created:
5 years, 3 months ago by Michael Lippautz Modified:
5 years, 3 months ago Reviewers:
Hannes Payer (out of office) CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@counters-2nd-try Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[heap] Introduce parallel compaction algorithm.
- The number of parallel tasks is still 1, i.e., we only compact on the main
thread.
- Remove emergency memory (PagedSpace, and CodeRange)
- Introduce partial compaction of pages.
- Logic for multiple tasks is in place.
BUG=chromium:524425
LOG=N
Committed: https://crrev.com/61ea4f55616d3f7bc2ce049a678f16f7475e03e0
Cr-Commit-Position: refs/heads/master@{#30787}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressed first round of comments #Patch Set 3 : Remove unused declarations of emergency memory related methods #Patch Set 4 : Rebase on master containing the counter modifications #Patch Set 5 : Fix case of aborted compaction #
Total comments: 8
Patch Set 6 : Addressed second round of comments #
Total comments: 16
Patch Set 7 : Addressed third round of comments #Patch Set 8 : Fix bug with deallocating the slotsbuffer chain #Patch Set 9 : Fix moving free list / merge compaction space dance #
Messages
Total messages: 22 (6 generated)
hpayer@chromium.org changed reviewers: + hpayer@chromium.org
First round of comments. https://codereview.chromium.org/1343333002/diff/1/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1343333002/diff/1/src/heap/mark-compact.cc#ne... src/heap/mark-compact.cc:3374: bool MarkCompactCollector::EvacuateLiveObjectsFromPageWithoutEmergency( I do not like the duplication of this piece of code here. Can we not model the emergency memory already differently? By adding memory to the compaction space? https://codereview.chromium.org/1343333002/diff/1/src/heap/mark-compact.cc#ne... src/heap/mark-compact.cc:3414: const int num_tasks = 1; This number will be configured. Make it a function call which currently just returns 1. https://codereview.chromium.org/1343333002/diff/1/src/heap/mark-compact.cc#ne... src/heap/mark-compact.cc:3428: // available or we fix comment line break. https://codereview.chromium.org/1343333002/diff/1/src/heap/mark-compact.cc#ne... src/heap/mark-compact.cc:3468: slots_buffer_allocator_->DeallocateChain(p->slots_buffer_address()); This is a strange case. We moved some objects and recorded slots for them. We do we throw away the slots buffers and rescan the pages for evacuation candidates? https://codereview.chromium.org/1343333002/diff/1/src/heap/mark-compact.cc#ne... src/heap/mark-compact.cc:3517: continue; Don't continue here, instead perform the action if the CAS succeeded in the true branch. https://codereview.chromium.org/1343333002/diff/1/src/heap/mark-compact.cc#ne... src/heap/mark-compact.cc:3787: if (FLAG_parallel_compaction) { It would be nice here to always compact in parallel, i.e. currently ToT would just use the main thread for compaction. --parallel-compaction should add more tasks if enabled (later). Can we do that already? https://codereview.chromium.org/1343333002/diff/1/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1343333002/diff/1/src/heap/spaces.cc#newcode2262 src/heap/spaces.cc:2262: void FreeList::Divide(FreeList** free_lists_to_fill, const int num) { I do not like this function. The linear complexity worries me, especially taking elements from the small list. Memory reducing GCs will have many free-list elements. Why don't we refill the free-lists on the fly (with synchronization of course)? And initialize it in the beginning just with huge and large chunks (a constant number). https://codereview.chromium.org/1343333002/diff/1/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1343333002/diff/1/src/heap/spaces.h#newcode278 src/heap/spaces.h:278: // compactor inspected it. Why do we need some many states? There is a list of evacuation candidates. Before compaction starts they should all be PENDING? After that they cas to COMPACTING. If compaction aborts they should change to ABORTED. WDYT?
Thanks PTAL; I also removed the unused code now as we agreed on switching to parallel-with-1-task per default. https://codereview.chromium.org/1343333002/diff/1/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1343333002/diff/1/src/heap/mark-compact.cc#ne... src/heap/mark-compact.cc:3374: bool MarkCompactCollector::EvacuateLiveObjectsFromPageWithoutEmergency( On 2015/09/16 09:35:41, Hannes Payer wrote: > I do not like the duplication of this piece of code here. Can we not model the > emergency memory already differently? By adding memory to the compaction space? As discussed offline, we get rid of the emergency space already in this CL. https://codereview.chromium.org/1343333002/diff/1/src/heap/mark-compact.cc#ne... src/heap/mark-compact.cc:3414: const int num_tasks = 1; On 2015/09/16 09:35:41, Hannes Payer wrote: > This number will be configured. Make it a function call which currently just > returns 1. Done. https://codereview.chromium.org/1343333002/diff/1/src/heap/mark-compact.cc#ne... src/heap/mark-compact.cc:3428: // available or we On 2015/09/16 09:35:41, Hannes Payer wrote: > fix comment line break. Done. https://codereview.chromium.org/1343333002/diff/1/src/heap/mark-compact.cc#ne... src/heap/mark-compact.cc:3468: slots_buffer_allocator_->DeallocateChain(p->slots_buffer_address()); On 2015/09/16 09:35:41, Hannes Payer wrote: > This is a strange case. We moved some objects and recorded slots for them. We do > we throw away the slots buffers and rescan the pages for evacuation candidates? This is not ideal (we recorded slots but we don't use them) but at least a sane starting point. I would suggest investigating the following in a followup: - Let's not unlink them. - Keep the flag marked as evacuation candidate. - Not mark it as to-be-rescanned. https://codereview.chromium.org/1343333002/diff/1/src/heap/mark-compact.cc#ne... src/heap/mark-compact.cc:3517: continue; On 2015/09/16 09:35:41, Hannes Payer wrote: > Don't continue here, instead perform the action if the CAS succeeded in the true > branch. Done. https://codereview.chromium.org/1343333002/diff/1/src/heap/mark-compact.cc#ne... src/heap/mark-compact.cc:3787: if (FLAG_parallel_compaction) { On 2015/09/16 09:35:41, Hannes Payer wrote: > It would be nice here to always compact in parallel, i.e. currently ToT would > just use the main thread for compaction. --parallel-compaction should add more > tasks if enabled (later). Can we do that already? Yep, that should work. Done. https://codereview.chromium.org/1343333002/diff/1/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1343333002/diff/1/src/heap/spaces.cc#newcode2262 src/heap/spaces.cc:2262: void FreeList::Divide(FreeList** free_lists_to_fill, const int num) { On 2015/09/16 09:35:41, Hannes Payer wrote: > I do not like this function. The linear complexity worries me, especially taking > elements from the small list. Memory reducing GCs will have many free-list > elements. > > Why don't we refill the free-lists on the fly (with synchronization of course)? > And initialize it in the beginning just with huge and large chunks (a constant > number). As discussed offline, we will not call it for now. Should we remove it completely? https://codereview.chromium.org/1343333002/diff/1/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1343333002/diff/1/src/heap/spaces.h#newcode278 src/heap/spaces.h:278: // compactor inspected it. On 2015/09/16 09:35:41, Hannes Payer wrote: > Why do we need some many states? > There is a list of evacuation candidates. Before compaction starts they should > all be PENDING? After that they cas to COMPACTING. If compaction aborts they > should change to ABORTED. WDYT? As discussed offline, flags can stay to check sanity of racing on the evacuation candidates list.
Getting there... https://codereview.chromium.org/1343333002/diff/80001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1343333002/diff/80001/src/heap/mark-compact.c... src/heap/mark-compact.cc:3375: compaction_spaces_for_tasks[i] = new CompactionSpaceCollection(heap()); Let's move the free list entries to the compaction space of the main thread. Otherwise, we change ToT memory usage. https://codereview.chromium.org/1343333002/diff/80001/src/heap/mark-compact.c... src/heap/mark-compact.cc:3394: // Merge back the compacted memory. Make this comment more precise, what is merged exactly. https://codereview.chromium.org/1343333002/diff/80001/src/heap/mark-compact.c... src/heap/mark-compact.cc:3415: // evacuate the page. This is not correct, we have to process both the slots buffer and the page by scanning it for evacuation. https://codereview.chromium.org/1343333002/diff/80001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1343333002/diff/80001/src/heap/spaces.cc#newc... src/heap/spaces.cc:2213: void FreeList::Divide(FreeList** free_lists_to_fill, const int num) { We will do that differently in a follow-up CL. Let's remove it.
PTAL https://codereview.chromium.org/1343333002/diff/80001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1343333002/diff/80001/src/heap/mark-compact.c... src/heap/mark-compact.cc:3375: compaction_spaces_for_tasks[i] = new CompactionSpaceCollection(heap()); On 2015/09/16 12:46:24, Hannes Payer wrote: > Let's move the free list entries to the compaction space of the main thread. > Otherwise, we change ToT memory usage. Done. https://codereview.chromium.org/1343333002/diff/80001/src/heap/mark-compact.c... src/heap/mark-compact.cc:3394: // Merge back the compacted memory. On 2015/09/16 12:46:24, Hannes Payer wrote: > Make this comment more precise, what is merged exactly. Done. https://codereview.chromium.org/1343333002/diff/80001/src/heap/mark-compact.c... src/heap/mark-compact.cc:3415: // evacuate the page. On 2015/09/16 12:46:24, Hannes Payer wrote: > This is not correct, we have to process both the slots buffer and the page by > scanning it for evacuation. Ugh... It should be fixed now (tm). https://codereview.chromium.org/1343333002/diff/80001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1343333002/diff/80001/src/heap/spaces.cc#newc... src/heap/spaces.cc:2213: void FreeList::Divide(FreeList** free_lists_to_fill, const int num) { On 2015/09/16 12:46:24, Hannes Payer wrote: > We will do that differently in a follow-up CL. Let's remove it. Done.
looking good https://codereview.chromium.org/1343333002/diff/100001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1343333002/diff/100001/src/heap/mark-compact.... src/heap/mark-compact.cc:3422: // - Leave the slots buffer there for processing of write-barriered entries added by the write barrier https://codereview.chromium.org/1343333002/diff/100001/src/heap/mark-compact.... src/heap/mark-compact.cc:3424: // - Rescan the page as we slot recording in the migration buffer only -we https://codereview.chromium.org/1343333002/diff/100001/src/heap/mark-compact.... src/heap/mark-compact.cc:3436: case MemoryChunk::kNoEvacuationCandidate: In this case DCHECK Page::RESCAN_ON_EVACUATION should hold. Please add this check. https://codereview.chromium.org/1343333002/diff/100001/src/heap/mark-compact.... src/heap/mark-compact.cc:3490: MemoryChunk::kNoEvacuationCandidate); Let's get rid of the kNoEvacuationCandidate case and set it back to done. Add an assert in the finalization code to check for the correct state of popular pages. https://codereview.chromium.org/1343333002/diff/100001/src/heap/mark-compact.... src/heap/mark-compact.cc:3747: // a compacted page is not completely free but has only been partially ... where a page was only partially compacted. https://codereview.chromium.org/1343333002/diff/100001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/1343333002/diff/100001/src/heap/mark-compact.... src/heap/mark-compact.h:723: int NumberOfParallelCompactionTasks() { return 1; } Let's add a todo here that this will be replaced with some smartness :) https://codereview.chromium.org/1343333002/diff/100001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1343333002/diff/100001/src/heap/spaces.cc#new... src/heap/spaces.cc:1029: // emergency_memory_ remove emergency_memory_ https://codereview.chromium.org/1343333002/diff/100001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1343333002/diff/100001/src/heap/spaces.h#newc... src/heap/spaces.h:272: // |kCompactingFinalize|: Parallel compaction is done but the chunk needs to Order.
https://codereview.chromium.org/1343333002/diff/100001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1343333002/diff/100001/src/heap/mark-compact.... src/heap/mark-compact.cc:3422: // - Leave the slots buffer there for processing of write-barriered On 2015/09/16 14:07:30, Hannes Payer wrote: > entries added by the write barrier Done. https://codereview.chromium.org/1343333002/diff/100001/src/heap/mark-compact.... src/heap/mark-compact.cc:3424: // - Rescan the page as we slot recording in the migration buffer only On 2015/09/16 14:07:30, Hannes Payer wrote: > -we Done. https://codereview.chromium.org/1343333002/diff/100001/src/heap/mark-compact.... src/heap/mark-compact.cc:3436: case MemoryChunk::kNoEvacuationCandidate: On 2015/09/16 14:07:30, Hannes Payer wrote: > In this case DCHECK Page::RESCAN_ON_EVACUATION should hold. Please add this > check. Done. https://codereview.chromium.org/1343333002/diff/100001/src/heap/mark-compact.... src/heap/mark-compact.cc:3490: MemoryChunk::kNoEvacuationCandidate); On 2015/09/16 14:07:30, Hannes Payer wrote: > Let's get rid of the kNoEvacuationCandidate case and set it back to done. Add an > assert in the finalization code to check for the correct state of popular pages. Done. https://codereview.chromium.org/1343333002/diff/100001/src/heap/mark-compact.... src/heap/mark-compact.cc:3747: // a compacted page is not completely free but has only been partially On 2015/09/16 14:07:30, Hannes Payer wrote: > ... where a page was only partially compacted. Done. https://codereview.chromium.org/1343333002/diff/100001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/1343333002/diff/100001/src/heap/mark-compact.... src/heap/mark-compact.h:723: int NumberOfParallelCompactionTasks() { return 1; } On 2015/09/16 14:07:30, Hannes Payer wrote: > Let's add a todo here that this will be replaced with some smartness :) Done. https://codereview.chromium.org/1343333002/diff/100001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1343333002/diff/100001/src/heap/spaces.cc#new... src/heap/spaces.cc:1029: // emergency_memory_ On 2015/09/16 14:07:30, Hannes Payer wrote: > remove emergency_memory_ Done. https://codereview.chromium.org/1343333002/diff/100001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1343333002/diff/100001/src/heap/spaces.h#newc... src/heap/spaces.h:272: // |kCompactingFinalize|: Parallel compaction is done but the chunk needs to On 2015/09/16 14:07:30, Hannes Payer wrote: > Order. Done.
lgtm
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/patch-status/1343333002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1343333002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/9675)
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/patch-status/1343333002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1343333002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by mlippautz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1343333002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1343333002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/61ea4f55616d3f7bc2ce049a678f16f7475e03e0 Cr-Commit-Position: refs/heads/master@{#30787}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/1347873003/ by mlippautz@chromium.org. The reason for reverting is: Check failed: https://chromegw.corp.google.com/i/client.v8/builders/V8%20Win64/builds/5535/.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
