Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1380723002:
[heap] Remove unswept bytes counter (Closed)
|
Created:
5 years, 2 months ago by Michael Lippautz Modified:
5 years, 2 months ago CC:
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] Remove unswept bytes counter
This change removes the unswept free bytes counter.
The new approach
- directly decrements allocated memory and capacity before sweeping (using live
bytes from the marker), and
- adds back capacity during refilling a free list.
This is another pre-work for moving around free lists while keeping the counters
in a sane state.
The previous approach allowed us to nail down how much memory is to-be-swept.
However, there were no users of this as we only used it do decrement it from
allocated memory (which still accounted for dead objects). If we want to keep
track of unswept free bytes in a space during compaction we can introduce a
separate new concurrent counter for this purpose.
BUG=chromium:524425
LOG=N
Committed: https://crrev.com/93e939837d53a3eadde8f049f1e87f412adb0050
Cr-Commit-Position: refs/heads/master@{#31175}
Patch Set 1 #Patch Set 2 : Fix accounting from mutator #Patch Set 3 : Properly increase and decrease capacity #Patch Set 4 : Simplify concatenate API and add comment #
Total comments: 3
Patch Set 5 : Addressed comment #Patch Set 6 : Fix compilation #
Total comments: 6
Patch Set 7 : Add separate accounting for committed memory #
Total comments: 4
Patch Set 8 : Rebase and remove handling of committed memory #Patch Set 9 : Fix accounting before and in ReleasePage() #
Total comments: 2
Patch Set 10 : Added CHECK #
Messages
Total messages: 43 (18 generated)
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/1380723002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1380723002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) 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/1380723002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1380723002/60001
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/1380723002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1380723002/80001
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/1380723002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1380723002/100001
mlippautz@chromium.org changed reviewers: + hpayer@chromium.org, ulan@chromium.org
PTAL, this is the counter change we talked about. We still rely on the marker keeping track of allocated bytes, but can get rid of the unswept bytes counter. In case we are interested in sweeper progress on byte level we can add a separate counter that keeps track of this. https://codereview.chromium.org/1380723002/diff/100001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1380723002/diff/100001/src/heap/mark-compact.... src/heap/mark-compact.cc:606: intptr_t added = space->free_list()->Concatenate(free_list); It should be relatively easy to make RefillFreeList work with compaction spaces now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, are there test for the counters? https://codereview.chromium.org/1380723002/diff/100001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1380723002/diff/100001/src/heap/mark-compact.... src/heap/mark-compact.cc:4390: space->accounting_stats_.DeallocateBytes(to_sweep); Use ShrinkSpace here?
There's 1 explicit test in test-heap.cc (TestSizeOfObjects) and several others that test the behavior implicitly by relying on SizeOfObjects(). https://codereview.chromium.org/1380723002/diff/100001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1380723002/diff/100001/src/heap/mark-compact.... src/heap/mark-compact.cc:4390: space->accounting_stats_.DeallocateBytes(to_sweep); On 2015/10/01 15:57:28, ulan wrote: > Use ShrinkSpace here? 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/1380723002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1380723002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel/builds/10240) v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_win_compile_dbg on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/8177)
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/1380723002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1380723002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1380723002/diff/140001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1380723002/diff/140001/src/heap/mark-compact.... src/heap/mark-compact.cc:4355: space->accounting_stats_.AllocateBytes(p->area_size()); This seems to be non-logical. We do we account here for allocated bytes, which are not allocated. https://codereview.chromium.org/1380723002/diff/140001/src/heap/mark-compact.... src/heap/mark-compact.cc:4390: space->accounting_stats_.ShrinkSpace(to_sweep); Here we are going to shrink the capacity, a lot. Almost every page will be selected for concurrent sweeping resulting in a very small capacity. Our tools may report a completely wrong picture. CommittedMemory() reports this counter to the statistics extensions.
Thanks a lot for thinking this through offline. PTAL. https://codereview.chromium.org/1380723002/diff/140001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1380723002/diff/140001/src/heap/mark-compact.... src/heap/mark-compact.cc:4355: space->accounting_stats_.AllocateBytes(p->area_size()); On 2015/10/02 06:52:09, Hannes Payer wrote: > This seems to be non-logical. We do we account here for allocated bytes, which > are not allocated. I agree, the change here is mechanical though. ReleasePage has a path where it subtracts the appropriate amount of bytes. I would rather try to fix the accounting itself in a separate CL and keep the changes to the swept bytes counter mechanical here. https://codereview.chromium.org/1380723002/diff/140001/src/heap/mark-compact.... src/heap/mark-compact.cc:4390: space->accounting_stats_.ShrinkSpace(to_sweep); On 2015/10/02 06:52:09, Hannes Payer wrote: > Here we are going to shrink the capacity, a lot. Almost every page will be > selected for concurrent sweeping resulting in a very small capacity. Our tools > may report a completely wrong picture. CommittedMemory() reports this counter to > the statistics extensions. I changed Committed() and MaxCommitted() to be based on an actual committed bytes counters. This way we - have accurate capacity/size/waste throughout sweeping, where capacity is the actually usable memory in a space (even during sweeping). - can later on extend on borrowing memory in RefillFreeList. - have accurate accounting for committed memory, including page headers. We probably get worse fragmentation as we so far ignored the page headers. Note that there is no invariant that committed >= capacity, as we could borrow memory. (Also, fragmentation during compaction could be quite high as CompactionSpaces are not considered for overall statistics.) WDYT?
https://codereview.chromium.org/1380723002/diff/140001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1380723002/diff/140001/src/heap/mark-compact.... src/heap/mark-compact.cc:4355: space->accounting_stats_.AllocateBytes(p->area_size()); On 2015/10/02 13:04:35, Michael Lippautz wrote: > On 2015/10/02 06:52:09, Hannes Payer wrote: > > This seems to be non-logical. We do we account here for allocated bytes, which > > are not allocated. > > I agree, the change here is mechanical though. ReleasePage has a path where it > subtracts the appropriate amount of bytes. > > I would rather try to fix the accounting itself in a separate CL and keep the > changes to the swept bytes counter mechanical here. The logic is just broken Let's fix it right away. https://codereview.chromium.org/1380723002/diff/160001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1380723002/diff/160001/src/heap/spaces.cc#new... src/heap/spaces.cc:1096: accounting_stats_.CommitMemory(size); NewSpace still reports not the right committed amount of memory. Maybe we should fix the commit memory reporting in a different cl. WDYT? https://codereview.chromium.org/1380723002/diff/160001/src/heap/spaces.cc#new... src/heap/spaces.cc:1165: accounting_stats_.CommitMemory(page->size()); Uncommit?
Ulan can you have another look since Hannes is on vacation? Committed accounting has landed in https://codereview.chromium.org/1388383002/ As capacity and committed accounting is now separated, I don't see any problems with reporting Chrome anymore. https://codereview.chromium.org/1380723002/diff/140001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1380723002/diff/140001/src/heap/mark-compact.... src/heap/mark-compact.cc:4355: space->accounting_stats_.AllocateBytes(p->area_size()); On 2015/10/05 15:23:26, Hannes Payer wrote: > On 2015/10/02 13:04:35, Michael Lippautz wrote: > > On 2015/10/02 06:52:09, Hannes Payer wrote: > > > This seems to be non-logical. We do we account here for allocated bytes, > which > > > are not allocated. > > > > I agree, the change here is mechanical though. ReleasePage has a path where it > > subtracts the appropriate amount of bytes. > > > > I would rather try to fix the accounting itself in a separate CL and keep the > > changes to the swept bytes counter mechanical here. > > The logic is just broken Let's fix it right away. Done. I removed the corresponding parts. ReleasePage() has 2 callers, and the accounting was relevant for the case where we have not swept the page. https://codereview.chromium.org/1380723002/diff/160001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1380723002/diff/160001/src/heap/spaces.cc#new... src/heap/spaces.cc:1096: accounting_stats_.CommitMemory(size); On 2015/10/05 15:23:26, Hannes Payer wrote: > NewSpace still reports not the right committed amount of memory. Maybe we should > fix the commit memory reporting in a different cl. WDYT? Obsolete, as committed accounting is not present in this CL anymore. https://codereview.chromium.org/1380723002/diff/160001/src/heap/spaces.cc#new... src/heap/spaces.cc:1165: accounting_stats_.CommitMemory(page->size()); On 2015/10/05 15:23:26, Hannes Payer wrote: > Uncommit? Done. Committed accounting landed in https://codereview.chromium.org/1388383002/
lgtm https://codereview.chromium.org/1380723002/diff/200001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1380723002/diff/200001/src/heap/mark-compact.... src/heap/mark-compact.cc:3862: p->ResetLiveBytes(); Please add check that the page has the WasSwept flag on.
https://codereview.chromium.org/1380723002/diff/200001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1380723002/diff/200001/src/heap/mark-compact.... src/heap/mark-compact.cc:3862: p->ResetLiveBytes(); On 2015/10/07 14:03:01, ulan wrote: > Please add check that the page has the WasSwept flag on. 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/1380723002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1380723002/220001
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 ulan@chromium.org Link to the patchset: https://codereview.chromium.org/1380723002/#ps220001 (title: "Added CHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1380723002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1380723002/220001
Message was sent while issue was closed.
Committed patchset #10 (id:220001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/93e939837d53a3eadde8f049f1e87f412adb0050 Cr-Commit-Position: refs/heads/master@{#31175} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
