Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1871423002:
[heap] Refactor Sweeper (Closed)
|
Created:
4 years, 8 months ago by Michael Lippautz Modified:
4 years, 8 months ago Reviewers:
Hannes Payer (out of office) 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] Refactor Sweeper
- Additionally allow to commit late lists to an already started sweeper
BUG=chromium:581412
LOG=N
Committed: https://crrev.com/18d92181fb2a0714c4fc4b5c174ffcd2a4ef7680
Cr-Commit-Position: refs/heads/master@{#35432}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : Fix uninitialized memory #Patch Set 7 : #
Total comments: 4
Patch Set 8 : One more API change useful for future changes #Patch Set 9 : Fix typo #
Total comments: 18
Patch Set 10 : Addressed comments #
Total comments: 6
Patch Set 11 : Comments round 2 #
Messages
Total messages: 34 (17 generated)
Patchset #4 (id:60001) has been deleted
Description was changed from ========== [heap] Refactor Sweeper. - Additionally allow to commit late lists that are also swept. BUG= ========== to ========== [heap] Refactor Sweeper. - Additionally allow to commit late lists to an already started sweeper BUG= ==========
Description was changed from ========== [heap] Refactor Sweeper. - Additionally allow to commit late lists to an already started sweeper BUG= ========== to ========== [heap] Refactor Sweeper - Additionally allow to commit late lists to an already started sweeper BUG= ==========
Description was changed from ========== [heap] Refactor Sweeper - Additionally allow to commit late lists to an already started sweeper BUG= ========== to ========== [heap] Refactor Sweeper - Additionally allow to commit late lists to an already started sweeper 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/1871423002/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871423002/130001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/4077)
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/1871423002/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871423002/130001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/13457)
mlippautz@chromium.org changed reviewers: + hpayer@chromium.org
ptal
https://codereview.chromium.org/1871423002/diff/130001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/1871423002/diff/130001/src/heap/mark-compact.... src/heap/mark-compact.h:403: class Sweeper { Sweeper is now untangled from MC and could in theory be safely moved to its own file. https://codereview.chromium.org/1871423002/diff/130001/src/heap/mark-compact.... src/heap/mark-compact.h:412: typedef std::vector<Page*> SweepingList; SweepingList and SweptList could in theory be the same, e.g. std::vector. (We would like to use std::sort for SweepingList) https://codereview.chromium.org/1871423002/diff/130001/src/heap/mark-compact.... src/heap/mark-compact.h:418: static int UnmanagedSweep(PagedSpace* space, Page* p, ObjectVisitor* v); UnmanagedSweep can still be used, but it should be clear that accounting needs to be done manually. https://codereview.chromium.org/1871423002/diff/130001/src/heap/mark-compact.... src/heap/mark-compact.h:461: SweepingList* late_sweeping_list_[kAllocationSpaces]; Not sure about the naming here. Late pages are pages added after sweeping has been started. WDYT? Also, I used AllocationSpace to index into the arrays, although we don't use NEW_SPACE and LO_SPACE. (NEW_SPACE will be relevant soon.)
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/1871423002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871423002/170001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I like where this is going. A couple of comments. https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.cc:504: StartSweepingHelper(OLD_SPACE); Instead of calling e.g. StartSweepingHelper 3 times with different enum values, we should just build a proper loop and iterate over the spaces we care about. This applies to the code below, above, and other pieces in that CL. https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.cc:3638: int max_pages) { max pages is a heuristic I do not really like, but it should be respected below. https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.cc:3645: late_list = late_sweeping_list_[identity]; Can we make a late sweeping list getter that takes the lock? https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.cc:3647: if ((required_freed_bytes > 0 && max_freed >= required_freed_bytes) && if max_freed >= required_freed_bytes we were successful and do not have to sweep more pages. https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.cc:3717: PrepareAddPage(space, page); You can also DCHECK here for DCHECK(sweeping_in_progress_); https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.cc:3724: void MarkCompactCollector::Sweeper::PrepareAddPage(AllocationSpace space, How about PrepareToBeSweptPage or something like that? https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.cc:3840: void MarkCompactCollector::Sweeper::ParallelSweepSpacesComplete() { Let's just fold that code into its caller. I do not like the name of that function and it is not necessary to have it. https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.h:418: static int UnmanagedSweep(PagedSpace* space, Page* p, ObjectVisitor* v); Raw* https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.h:451: static const int kAllocationSpaces = 5; LO_SPACE + 1
Thanks! Addressed all comments. https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.cc:504: StartSweepingHelper(OLD_SPACE); On 2016/04/12 at 10:07:32, Hannes Payer wrote: > Instead of calling e.g. StartSweepingHelper 3 times with different enum values, we should just build a proper loop and iterate over the spaces we care about. This applies to the code below, above, and other pieces in that CL. Done. https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.cc:3638: int max_pages) { On 2016/04/12 at 10:07:32, Hannes Payer wrote: > max pages is a heuristic I do not really like, but it should be respected below. Done, I redid the early bailouts for required_freed_bytes and max_pages. https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.cc:3645: late_list = late_sweeping_list_[identity]; On 2016/04/12 at 10:07:32, Hannes Payer wrote: > Can we make a late sweeping list getter that takes the lock? Done: GetLateSweepingListSafe https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.cc:3647: if ((required_freed_bytes > 0 && max_freed >= required_freed_bytes) && On 2016/04/12 at 10:07:32, Hannes Payer wrote: > if max_freed >= required_freed_bytes we were successful and do not have to sweep more pages. Done (fixed condition). https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.cc:3717: PrepareAddPage(space, page); On 2016/04/12 at 10:07:32, Hannes Payer wrote: > You can also DCHECK here for DCHECK(sweeping_in_progress_); Done. https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.cc:3724: void MarkCompactCollector::Sweeper::PrepareAddPage(AllocationSpace space, On 2016/04/12 at 10:07:32, Hannes Payer wrote: > How about PrepareToBeSweptPage or something like that? Done. https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.cc:3840: void MarkCompactCollector::Sweeper::ParallelSweepSpacesComplete() { On 2016/04/12 at 10:07:32, Hannes Payer wrote: > Let's just fold that code into its caller. I do not like the name of that function and it is not necessary to have it. Done. https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.h:418: static int UnmanagedSweep(PagedSpace* space, Page* p, ObjectVisitor* v); On 2016/04/12 at 10:07:32, Hannes Payer wrote: > Raw* Done. https://codereview.chromium.org/1871423002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.h:451: static const int kAllocationSpaces = 5; On 2016/04/12 at 10:07:32, Hannes Payer wrote: > LO_SPACE + 1 Done. I used LAST_PAGED_SPACE + 1 as you initially suggested. We will not sweeper LO_SPACE soon (but we will need NEW_SPACE).
lgtm with a few nits https://codereview.chromium.org/1871423002/diff/190001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1871423002/diff/190001/src/heap/mark-compact.... src/heap/mark-compact.cc:496: std::sort(sweeping_list_[OLD_SPACE].begin(), sweeping_list_[OLD_SPACE].end(), Can you also refactor this piece of repeating statements? https://codereview.chromium.org/1871423002/diff/190001/src/heap/mark-compact.... src/heap/mark-compact.cc:555: HelpSweepInParallel(OLD_SPACE, 0); Can you refactor also that piece to not repeat the statement? https://codereview.chromium.org/1871423002/diff/190001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/1871423002/diff/190001/src/heap/mark-compact.... src/heap/mark-compact.h:437: int HelpSweepInParallel(AllocationSpace identity, int required_freed_bytes, I find the overloading really confusing. Can we just say ParallelSweepSpace, ParallelSweepPage, ParallelSweepList?
https://codereview.chromium.org/1871423002/diff/190001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1871423002/diff/190001/src/heap/mark-compact.... src/heap/mark-compact.cc:496: std::sort(sweeping_list_[OLD_SPACE].begin(), sweeping_list_[OLD_SPACE].end(), On 2016/04/12 16:26:11, Hannes Payer wrote: > Can you also refactor this piece of repeating statements? Done. https://codereview.chromium.org/1871423002/diff/190001/src/heap/mark-compact.... src/heap/mark-compact.cc:555: HelpSweepInParallel(OLD_SPACE, 0); On 2016/04/12 16:26:11, Hannes Payer wrote: > Can you refactor also that piece to not repeat the statement? Done. https://codereview.chromium.org/1871423002/diff/190001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/1871423002/diff/190001/src/heap/mark-compact.... src/heap/mark-compact.h:437: int HelpSweepInParallel(AllocationSpace identity, int required_freed_bytes, On 2016/04/12 16:26:11, Hannes Payer wrote: > I find the overloading really confusing. Can we just say ParallelSweepSpace, > ParallelSweepPage, ParallelSweepList? 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/patch-status/1871423002/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871423002/210001
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/1871423002/#ps210001 (title: "Comments round 2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1871423002/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871423002/210001
Message was sent while issue was closed.
Description was changed from ========== [heap] Refactor Sweeper - Additionally allow to commit late lists to an already started sweeper BUG=chromium:581412 LOG=N ========== to ========== [heap] Refactor Sweeper - Additionally allow to commit late lists to an already started sweeper BUG=chromium:581412 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #11 (id:210001)
Message was sent while issue was closed.
Description was changed from ========== [heap] Refactor Sweeper - Additionally allow to commit late lists to an already started sweeper BUG=chromium:581412 LOG=N ========== to ========== [heap] Refactor Sweeper - Additionally allow to commit late lists to an already started sweeper BUG=chromium:581412 LOG=N Committed: https://crrev.com/18d92181fb2a0714c4fc4b5c174ffcd2a4ef7680 Cr-Commit-Position: refs/heads/master@{#35432} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/18d92181fb2a0714c4fc4b5c174ffcd2a4ef7680 Cr-Commit-Position: refs/heads/master@{#35432} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
