Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1863983002:
🏄 [heap] Add page evacuation mode for new->old (Closed)
|
Created:
4 years, 8 months ago by Michael Lippautz 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 page evacuation mode for new->old
In a full mark-compact GC, instead of copying memory to old space for
pages that have more than X% live bytes, we just move the whole page over to old
space.
X=70 (default value)
BUG=chromium:581412
LOG=N
Committed: https://crrev.com/0d7e23a6edd3822970983030a77a5b80cd337911
Cr-Commit-Position: refs/heads/master@{#35610}
Patch Set 1 #Patch Set 2 : Add sweeping (on main thread); fix ArrayBufferTracker; fix ExternalStringTable #Patch Set 3 : Add Page::Convert and rebase #Patch Set 4 : Fixing external string table and ArrayBufferTracker #Patch Set 5 : Lower fast evacuation threshold to 0% to get test coverage #Patch Set 6 : Properly unlink categories before sweeping #Patch Set 7 : Fix race condition when adding a page to swept list #Patch Set 8 : Page moving phase needs to be performed before parallel evacuation #Patch Set 9 : Concurrent sweeping for FAST_EVACUATION pages #Patch Set 10 : Cleanup code #Patch Set 11 : Add promoted size #Patch Set 12 : Fix one more race condition #Patch Set 13 : Cleanup #
Total comments: 12
Patch Set 14 : Addressed first round of comments #Patch Set 15 : Some more optimizations #Patch Set 16 : Rebase on master #
Total comments: 9
Patch Set 17 : Addressed comments #Patch Set 18 : Rebase on master #Patch Set 19 : Add test for promoting pages below age mark #Patch Set 20 : Fix compilation of test on Windows and set flag to its default (70) #Patch Set 21 : Disable optimize_for_size for the feature test as we require >1 page new space #
Messages
Total messages: 83 (53 generated)
Description was changed from ========== [heap] Add fast evacuation mode from new->old Instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. BUG= ========== to ========== [heap] Add fast evacuation mode from new->old Instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. BUG=chromium:581412 LOG=N ==========
Description was changed from ========== [heap] Add fast evacuation mode from new->old Instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. BUG=chromium:581412 LOG=N ========== to ========== [heap] Add fast evacuation mode from new -> old generation In a full mark-compact GC cycle, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. BUG=chromium:581412 LOG=N ==========
Description was changed from ========== [heap] Add fast evacuation mode from new -> old generation In a full mark-compact GC cycle, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. BUG=chromium:581412 LOG=N ========== to ========== [heap] Add fast evacuation mode from new -> old generation In a full mark-compact GC cycle, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. BUG=chromium:581412 LOG=N ==========
Description was changed from ========== [heap] Add fast evacuation mode from new -> old generation In a full mark-compact GC cycle, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. BUG=chromium:581412 LOG=N ========== to ========== [heap] Add fast evacuation mode from new to old generation In a full mark-compact GC cycle, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. BUG=chromium:581412 LOG=N ==========
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Description was changed from ========== [heap] Add fast evacuation mode from new to old generation In a full mark-compact GC cycle, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. BUG=chromium:581412 LOG=N ========== to ========== [heap] Add fast evacuation mode from new to old generation In a full mark-compact GC cycle, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. X=90 (default value) BUG=chromium:581412 LOG=N ==========
Patchset #7 (id:150001) has been deleted
Patchset #7 (id:170001) has been deleted
Description was changed from ========== [heap] Add fast evacuation mode from new to old generation In a full mark-compact GC cycle, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. X=90 (default value) BUG=chromium:581412 LOG=N ========== to ========== [heap] Add page evacuation mode from new to old generation In a full mark-compact GC cycle, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. X=90 (default value) BUG=chromium:581412 LOG=N ==========
Patchset #9 (id:230001) has been deleted
Description was changed from ========== [heap] Add page evacuation mode from new to old generation In a full mark-compact GC cycle, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. X=90 (default value) BUG=chromium:581412 LOG=N ========== to ========== [heap] Add page evacuation mode for new->old In a full mark-compact GC cycle, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. X=90 (default value) BUG=chromium:581412 LOG=N ==========
Description was changed from ========== [heap] Add page evacuation mode for new->old In a full mark-compact GC cycle, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. X=90 (default value) BUG=chromium:581412 LOG=N ========== to ========== [heap] Add page evacuation mode for new->old In a full mark-compact GC, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. X=90 (default value) BUG=chromium:581412 LOG=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/patch-status/1863983002/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863983002/290001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #12 (id:310001) has been deleted
Patchset #13 (id:350001) has been deleted
Patchset #13 (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/patch-status/1863983002/390001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863983002/390001
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/1863983002/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863983002/410001
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/patch-status/1863983002/430001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863983002/430001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #16 (id:450001) has been deleted
Patchset #16 (id:470001) has been deleted
Patchset #7 (id:190001) has been deleted
Patchset #7 (id:210001) has been deleted
mlippautz@chromium.org changed reviewers: + hpayer@chromium.org, ulan@chromium.org
Please take a look. Not going to land before the branch cut. https://codereview.chromium.org/1863983002/diff/490001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/1863983002/diff/490001/src/flag-definitions.h... src/flag-definitions.h:263: DEFINE_INT(page_evacuation_threshold, 0, Will set to 90 before landing. https://codereview.chromium.org/1863983002/diff/490001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/1863983002/diff/490001/src/heap/mark-compact.... src/heap/mark-compact.h:834: SweepingList* sweeping_list_shared_; Not too happy about cluttering up all the sweeper lists. Maybe we should create a SweeperState that contains all the lists and the corresponding mutex and getters?
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/1863983002/490001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863983002/490001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
First round of comments. https://codereview.chromium.org/1863983002/diff/490001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1863983002/diff/490001/src/heap/mark-compact.... src/heap/mark-compact.cc:1784: page->heap()->new_space()->ReplaceWithEmptyPage(page); Why don't we take care of the new space size at the end of the GC? Maybe we decide to shrink new space and don't need the extra page... WDYT? https://codereview.chromium.org/1863983002/diff/490001/src/heap/mark-compact.... src/heap/mark-compact.cc:3210: bool fast_evac_pages = false; Can you factor this block of code out into a method. This one is getting pretty long. https://codereview.chromium.org/1863983002/diff/490001/src/heap/mark-compact.... src/heap/mark-compact.cc:3221: space->UnlinkFreeListCategories(page); New space pages never have free lists. Can you DCHECK that this is the case. The code dealing with free lists here should not be necessary. https://codereview.chromium.org/1863983002/diff/490001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/1863983002/diff/490001/src/heap/mark-compact.... src/heap/mark-compact.h:834: SweepingList* sweeping_list_shared_; On 2016/04/08 at 07:09:58, Michael Lippautz wrote: > Not too happy about cluttering up all the sweeper lists. > > Maybe we should create a SweeperState that contains all the lists and the corresponding mutex and getters? Yeah, that does not really scale. You can do that in a follow-up cl. I don't like the name sweeping_list_shared_; The pages are promoted pages, let's call it like that. https://codereview.chromium.org/1863983002/diff/490001/test/cctest/heap/test-... File test/cctest/heap/test-heap.cc (right): https://codereview.chromium.org/1863983002/diff/490001/test/cctest/heap/test-... test/cctest/heap/test-heap.cc:3562: i::FLAG_page_evacuation_threshold = 101; That is not very nice... should we introduce a separate flag for that?
Patchset #15 (id:510001) has been deleted
Thanks for the feedback! All addressed. https://codereview.chromium.org/1863983002/diff/490001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1863983002/diff/490001/src/heap/mark-compact.... src/heap/mark-compact.cc:1784: page->heap()->new_space()->ReplaceWithEmptyPage(page); On 2016/04/08 10:42:40, Hannes Payer wrote: > Why don't we take care of the new space size at the end of the GC? Maybe we > decide to shrink new space and don't need the extra page... WDYT? As discussed offline: Let's keep it for now. We can unify growing/shrinking in another CL. https://codereview.chromium.org/1863983002/diff/490001/src/heap/mark-compact.... src/heap/mark-compact.cc:3210: bool fast_evac_pages = false; On 2016/04/08 10:42:40, Hannes Payer wrote: > Can you factor this block of code out into a method. This one is getting pretty > long. Done. https://codereview.chromium.org/1863983002/diff/490001/src/heap/mark-compact.... src/heap/mark-compact.cc:3221: space->UnlinkFreeListCategories(page); On 2016/04/08 10:42:40, Hannes Payer wrote: > New space pages never have free lists. Can you DCHECK that this is the case. The > code dealing with free lists here should not be necessary. Done. https://codereview.chromium.org/1863983002/diff/490001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/1863983002/diff/490001/src/heap/mark-compact.... src/heap/mark-compact.h:834: SweepingList* sweeping_list_shared_; On 2016/04/08 10:42:40, Hannes Payer wrote: > On 2016/04/08 at 07:09:58, Michael Lippautz wrote: > > Not too happy about cluttering up all the sweeper lists. > > > > Maybe we should create a SweeperState that contains all the lists and the > corresponding mutex and getters? > > Yeah, that does not really scale. You can do that in a follow-up cl. > > I don't like the name sweeping_list_shared_; The pages are promoted pages, let's > call it like that. It's sweeping_list_promoted_pages_ for now. https://codereview.chromium.org/1863983002/diff/490001/test/cctest/heap/test-... File test/cctest/heap/test-heap.cc (right): https://codereview.chromium.org/1863983002/diff/490001/test/cctest/heap/test-... test/cctest/heap/test-heap.cc:3562: i::FLAG_page_evacuation_threshold = 101; On 2016/04/08 10:42:40, Hannes Payer wrote: > That is not very nice... should we introduce a separate flag for that? Done.
Patchset #16 (id:540001) has been deleted
Patchset #16 (id:560001) 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/patch-status/1863983002/600001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863983002/600001
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/patch-status/1863983002/640001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863983002/640001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #16 (id:580001) has been deleted
Patchset #16 (id:600001) has been deleted
ready again
https://codereview.chromium.org/1863983002/diff/640001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/1863983002/diff/640001/src/flag-definitions.h... src/flag-definitions.h:256: DEFINE_BOOL(page_evacuation, true, "evacuate pages based on utilization") The term evacuation is already used in v8. Evacuation candidates are pages that will be compacted. We are not going to move objects when this mode is on, we are moving pages instead. Better name? -fast_page_evacuation -page_promotion WDYT? https://codereview.chromium.org/1863983002/diff/640001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1863983002/diff/640001/src/heap/mark-compact.... src/heap/mark-compact.cc:3054: if (!FLAG_page_evacuation) Invert this case: if (FLAG_page_evacuation) https://codereview.chromium.org/1863983002/diff/640001/src/heap/mark-compact.... src/heap/mark-compact.cc:3327: if (evacuated_pages) { This should be part of UpdatePointersAfterEvacuation. And there we already process the string table. Have a look how to integrate into that cleanup logic. https://codereview.chromium.org/1863983002/diff/640001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1863983002/diff/640001/src/heap/spaces.h#newc... src/heap/spaces.h:424: FAST_NEW_OLD_EVACUATION, Keep this page flag in sync with the regular flags.
https://codereview.chromium.org/1863983002/diff/640001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/1863983002/diff/640001/src/flag-definitions.h... src/flag-definitions.h:256: DEFINE_BOOL(page_evacuation, true, "evacuate pages based on utilization") On 2016/04/18 12:12:23, Hannes Payer wrote: > The term evacuation is already used in v8. Evacuation candidates are pages that > will be compacted. We are not going to move objects when this mode is on, we are > moving pages instead. > > Better name? > -fast_page_evacuation > -page_promotion > > WDYT? page_promotion it is https://codereview.chromium.org/1863983002/diff/640001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1863983002/diff/640001/src/heap/mark-compact.... src/heap/mark-compact.cc:3054: if (!FLAG_page_evacuation) On 2016/04/18 12:12:23, Hannes Payer wrote: > Invert this case: > if (FLAG_page_evacuation) Done. https://codereview.chromium.org/1863983002/diff/640001/src/heap/mark-compact.... src/heap/mark-compact.cc:3054: if (!FLAG_page_evacuation) On 2016/04/18 12:12:23, Hannes Payer wrote: > Invert this case: > if (FLAG_page_evacuation) Done. https://codereview.chromium.org/1863983002/diff/640001/src/heap/mark-compact.... src/heap/mark-compact.cc:3327: if (evacuated_pages) { On 2016/04/18 12:12:23, Hannes Payer wrote: > This should be part of UpdatePointersAfterEvacuation. And there we already > process the string table. Have a look how to integrate into that cleanup logic. Removed. As you said, this should fall out from regular pointers evacuation. https://codereview.chromium.org/1863983002/diff/640001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1863983002/diff/640001/src/heap/spaces.h#newc... src/heap/spaces.h:424: FAST_NEW_OLD_EVACUATION, On 2016/04/18 12:12:23, Hannes Payer wrote: > Keep this page flag in sync with the regular flags. Done.
Description was changed from ========== [heap] Add page evacuation mode for new->old In a full mark-compact GC, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. X=90 (default value) BUG=chromium:581412 LOG=N ========== to ========== [heap] Add page evacuation mode for new->old In a full mark-compact GC, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. X=70 (default value) BUG=chromium:581412 LOG=N ==========
LGTM Please at some tests, that show that the features works.
On 2016/04/19 07:24:40, Hannes Payer wrote: > LGTM > > Please at some tests, that show that the features works. Added a test that checks that a page below age mark and above the promotion threshold is properly moved to old space.
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/1863983002/700001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863983002/700001
Patchset #21 (id:720001) has been deleted
Patchset #7 (id:250001) 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/patch-status/1863983002/740001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863983002/740001
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/1863983002/760001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863983002/760001
Patchset #21 (id:760001) has been deleted
Patchset #21 (id:780001) 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/patch-status/1863983002/800001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863983002/800001
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 Link to the patchset: https://codereview.chromium.org/1863983002/#ps800001 (title: "Disable optimize_for_size for the feature test as we require >1 page new space")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1863983002/800001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863983002/800001
Message was sent while issue was closed.
Description was changed from ========== [heap] Add page evacuation mode for new->old In a full mark-compact GC, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. X=70 (default value) BUG=chromium:581412 LOG=N ========== to ========== [heap] Add page evacuation mode for new->old In a full mark-compact GC, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. X=70 (default value) BUG=chromium:581412 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #21 (id:800001)
Message was sent while issue was closed.
Description was changed from ========== [heap] Add page evacuation mode for new->old In a full mark-compact GC, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. X=70 (default value) BUG=chromium:581412 LOG=N ========== to ========== [heap] Add page evacuation mode for new->old In a full mark-compact GC, instead of copying memory to old space for pages that have more than X% live bytes, we just move the whole page over to old space. X=70 (default value) BUG=chromium:581412 LOG=N Committed: https://crrev.com/0d7e23a6edd3822970983030a77a5b80cd337911 Cr-Commit-Position: refs/heads/master@{#35610} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/0d7e23a6edd3822970983030a77a5b80cd337911 Cr-Commit-Position: refs/heads/master@{#35610}
Message was sent while issue was closed.
A revert of this CL (patchset #21 id:800001) has been created in https://codereview.chromium.org/1896883003/ by machenbach@chromium.org. The reason for reverting is: [Sheriff] Breaks: https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
