Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1382003002:
[heap] Divide available memory upon compaction tasks (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@counters Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[heap] Divide available memory upon compaction tasks
- Fairly (round-robin) divide available memory upon compaction tasks.
- Ensure an upper limit (of memory) since dividing is O(n) for n free-space
nodes.
- Refill from free lists managed by sweeper once a compaction space becomes
empty.
Assumption for dividing memory: Memory in the free lists is sparse upon starting
compaction (which means that only few nodes are available), except for memory
reducer GCs, which happen in idle time though (so it's less of a problem).
BUG=chromium:524425
LOG=N
Committed: https://crrev.com/30236c052ba9266fc55412a8fd63b17f683ff40b
Cr-Commit-Position: refs/heads/master@{#31234}
Patch Set 1 : Initial #Patch Set 2 : Rebase on ToT #Patch Set 3 : Shuffle around declarations and add HeapTester friend #Patch Set 4 : Rework tests #
Total comments: 10
Patch Set 5 : Addressed first round of comments #
Total comments: 1
Patch Set 6 : Rebase #Patch Set 7 : Rework RefillFreeList (now in *Space); Do not refill from small and medium free lists #
Total comments: 12
Patch Set 8 : Addressed comments #
Total comments: 2
Patch Set 9 : Rebase + addressed comment #
Messages
Total messages: 55 (27 generated)
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #3 (id:120001) 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/1382003002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1382003002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/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/patch-status/1382003002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1382003002/160001
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/10470)
Patchset #4 (id:160001) 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/1382003002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1382003002/180001
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/10472) v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...) v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
Patchset #4 (id:180001) 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/1382003002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1382003002/200001
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/10473)
Patchset #4 (id:200001) 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/1382003002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1382003002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...) v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
Patchset #4 (id:220001) 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/1382003002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1382003002/240001
mlippautz@chromium.org changed reviewers: + ulan@chromium.org
PTAL. Now that counters are separated we can properly share parts of free lists concurrently. Left-overs: - EnsureSweepingCompleted() is not ready for concurrency. https://codereview.chromium.org/1382003002/diff/240001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1382003002/diff/240001/src/heap/mark-compact.... src/heap/mark-compact.cc:3421: const intptr_t kWantedMemory = 500 * KB; We could branch in a fast case here for 1 compaction tasks: Just move over the free list (i.e. PageSpace::MoveOverMemory()). https://codereview.chromium.org/1382003002/diff/240001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1382003002/diff/240001/src/heap/spaces.cc#new... src/heap/spaces.cc:2449: // Give back left overs that were not required by {size_in_bytes}. This could potentially increase fragmentation, but would allow for more tasks to share a few huge nodes. During bencmarking (telemetry, smoothness) I found that creating more tasks speeds up the process almost linearly, but results in #tasks > #huge nodes, which potentially increases memory (as we need to Expand() in those tasks).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1382003002/diff/240001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1382003002/diff/240001/src/heap/mark-compact.... src/heap/mark-compact.cc:611: void MarkCompactCollector::RefillFreeList(CompactionSpace* space) { Will this function be used in other CL? https://codereview.chromium.org/1382003002/diff/240001/src/heap/mark-compact.... src/heap/mark-compact.cc:621: intptr_t kWantedMemory = 500 * KB; This constant is duplicated below. Let's unify them. Could you write a comment explaining why 500*KB? https://codereview.chromium.org/1382003002/diff/240001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1382003002/diff/240001/src/heap/spaces.cc#new... src/heap/spaces.cc:1014: for (CompactionSpace* space = other[index]->Get(identity()); Let's simplify the "for" statement so that the correctness is more obvious. Is it intended that we early exit from the loop as soon as one space is filled? If not, consider something like: while (there_is_free_space && not_all_spaces_filled) { for (int i = 0; i < num; i++) { try_fill_in(space[i]); } } https://codereview.chromium.org/1382003002/diff/240001/src/heap/spaces.cc#new... src/heap/spaces.cc:2437: FreeSpace* FreeList::TryRemoveMemory(intptr_t size_in_bytes) { Could you please comment the postconditions of this function? When node == nullptr is returned, how node.size relates to size_in_bytes.
https://codereview.chromium.org/1382003002/diff/240001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/1382003002/diff/240001/src/heap/mark-compact.... src/heap/mark-compact.cc:611: void MarkCompactCollector::RefillFreeList(CompactionSpace* space) { On 2015/10/09 11:48:48, ulan wrote: > Will this function be used in other CL? The function should already be used as PagedSpace::SlowAllocateRaw() calls into RefillFreeList() with {this} being a CompactionSpace. https://codereview.chromium.org/1382003002/diff/240001/src/heap/mark-compact.... src/heap/mark-compact.cc:621: intptr_t kWantedMemory = 500 * KB; On 2015/10/09 11:48:48, ulan wrote: > This constant is duplicated below. Let's unify them. > Unified as MarkCompactCollector::kCompactionMemoryWanted > Could you write a comment explaining why 500*KB? Nope :) It's an upper bound to somehow limit free list node traversal. A better constant would probably consider the number of compaction tasks and be smaller (?) https://codereview.chromium.org/1382003002/diff/240001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1382003002/diff/240001/src/heap/spaces.cc#new... src/heap/spaces.cc:1014: for (CompactionSpace* space = other[index]->Get(identity()); On 2015/10/09 11:48:48, ulan wrote: > Let's simplify the "for" statement so that the correctness is more obvious. > > Is it intended that we early exit from the loop as soon as one space is filled? > > If not, consider something like: > while (there_is_free_space && not_all_spaces_filled) { > for (int i = 0; i < num; i++) { > try_fill_in(space[i]); > } > } Done, rewrote the loop. The intention is summarized by your suggestion: keep going in dividing memory as long as there is either no memory available or all spaces are satisfied. https://codereview.chromium.org/1382003002/diff/240001/src/heap/spaces.cc#new... src/heap/spaces.cc:2437: FreeSpace* FreeList::TryRemoveMemory(intptr_t size_in_bytes) { On 2015/10/09 11:48:48, ulan wrote: > Could you please comment the postconditions of this function? > When node == nullptr is returned, how node.size relates to size_in_bytes. Done. I added a high-level description to the .h file. Also I renamed the parameter to hint_...., as the parameter is rather a hint than actually enforcing anything. https://codereview.chromium.org/1382003002/diff/260001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1382003002/diff/260001/src/heap/spaces.cc#new... src/heap/spaces.cc:2459: if (node == nullptr) node = FindNodeIn(kMedium, &node_size); Remove medium and small lists?
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/1382003002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1382003002/300001
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
+hpayer PTAL - RefillFreeList is part of PagedSpace and CompactionSpace to make use of polymorphism. (The other solution would require a branch or dynamic_cast<>, which should be avoided) - TryRemoveMemory now ignores small and medium-sized free list categories.
Are you going to refill the compaction space CLs on the fly when they run out of memory? https://codereview.chromium.org/1382003002/diff/300001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (left): https://codereview.chromium.org/1382003002/diff/300001/src/heap/mark-compact.... src/heap/mark-compact.cc:602: // to only refill them for the old space. The comment is not true: old space + code space + map space https://codereview.chromium.org/1382003002/diff/300001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/1382003002/diff/300001/src/heap/mark-compact.... src/heap/mark-compact.h:514: // Free lists filled by sweeper and consumed byh corresponding spaces by https://codereview.chromium.org/1382003002/diff/300001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1382003002/diff/300001/src/heap/spaces.cc#new... src/heap/spaces.cc:992: FreeSpace* space = free_list()->TryRemoveMemory(size_in_bytes); Call it free_space, since space is usually used in a different context. https://codereview.chromium.org/1382003002/diff/300001/src/heap/spaces.cc#new... src/heap/spaces.cc:1048: // to only refill them for the old space. Comment outdated. https://codereview.chromium.org/1382003002/diff/300001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1382003002/diff/300001/src/heap/spaces.h#newc... src/heap/spaces.h:1731: // size in the appropriate free list category. If no suitable node could be What is appropriate?
Ignore the refill comment, the overloading takes care of it... https://codereview.chromium.org/1382003002/diff/300001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1382003002/diff/300001/src/heap/spaces.cc#new... src/heap/spaces.cc:1065: UNREACHABLE(); Add comment.
LGTM after fixing the nits
https://codereview.chromium.org/1382003002/diff/300001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (left): https://codereview.chromium.org/1382003002/diff/300001/src/heap/mark-compact.... src/heap/mark-compact.cc:602: // to only refill them for the old space. On 2015/10/12 16:38:25, Hannes Payer wrote: > The comment is not true: old space + code space + map space Done. https://codereview.chromium.org/1382003002/diff/300001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/1382003002/diff/300001/src/heap/mark-compact.... src/heap/mark-compact.h:514: // Free lists filled by sweeper and consumed byh corresponding spaces On 2015/10/12 16:38:25, Hannes Payer wrote: > by Done. https://codereview.chromium.org/1382003002/diff/300001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1382003002/diff/300001/src/heap/spaces.cc#new... src/heap/spaces.cc:992: FreeSpace* space = free_list()->TryRemoveMemory(size_in_bytes); On 2015/10/12 16:38:25, Hannes Payer wrote: > Call it free_space, since space is usually used in a different context. Done. https://codereview.chromium.org/1382003002/diff/300001/src/heap/spaces.cc#new... src/heap/spaces.cc:1048: // to only refill them for the old space. On 2015/10/12 16:38:25, Hannes Payer wrote: > Comment outdated. Done. https://codereview.chromium.org/1382003002/diff/300001/src/heap/spaces.cc#new... src/heap/spaces.cc:1065: UNREACHABLE(); On 2015/10/12 16:42:33, Hannes Payer wrote: > Add comment. Done. https://codereview.chromium.org/1382003002/diff/300001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1382003002/diff/300001/src/heap/spaces.h#newc... src/heap/spaces.h:1731: // size in the appropriate free list category. If no suitable node could be On 2015/10/12 16:38:25, Hannes Payer wrote: > What is appropriate? Done.
lgtm https://codereview.chromium.org/1382003002/diff/320001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1382003002/diff/320001/src/heap/spaces.h#newc... src/heap/spaces.h:1732: // node could be found, the method falls back to retrieving a {FreeSpace} not Remove "not" from "a {FreeSpace} not"
https://codereview.chromium.org/1382003002/diff/320001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1382003002/diff/320001/src/heap/spaces.h#newc... src/heap/spaces.h:1732: // node could be found, the method falls back to retrieving a {FreeSpace} not On 2015/10/13 10:01:23, ulan wrote: > Remove "not" from "a {FreeSpace} not" Done.
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/1382003002/#ps340001 (title: "Rebase + addressed comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1382003002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1382003002/340001
Message was sent while issue was closed.
Committed patchset #9 (id:340001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/30236c052ba9266fc55412a8fd63b17f683ff40b Cr-Commit-Position: refs/heads/master@{#31234}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:340001) has been created in https://codereview.chromium.org/1406533002/ by mlippautz@chromium.org. The reason for reverting is: Failing tests: https://chromegw.corp.google.com/i/client.v8/builders/V8%20Linux%20-%20arm64%.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
