Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2209583002:
[heap] Remove border page (Closed)
|
Created:
4 years, 4 months ago by Michael Lippautz Modified:
4 years, 2 months ago CC:
v8-reviews_googlegroups.com, Hannes Payer (out of office), ulan Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[heap] Remove border page
A page now belongs either the nursery *or* the intermediate gen. The page that
contained objects of both spaces is removed in this change.
BUG=chromium:636331
Committed: https://crrev.com/42ece47446f0dbd3779d6e0e00dce97a1931a9f9
Cr-Commit-Position: refs/heads/master@{#39778}
Patch Set 1 #Patch Set 2 : Remove age_mark_ field and accessors. #Patch Set 3 : rebase #Patch Set 4 : Fix AllocatedSinceLastGC when top() is just behind the page #Patch Set 5 : Fixes fixes fixes... #Patch Set 6 : Fixes #Patch Set 7 : Rebase #Patch Set 8 : Cleanup #Patch Set 9 : Fix acconting for fragmentation #
Total comments: 8
Patch Set 10 : Allow empty intermediate generation #
Messages
Total messages: 54 (45 generated)
Description was changed from ========== [heap] Remove border page - Age is now specified solely by the page itself. - Upon setting an age mark, we create a filler so that subsequent allocations target the next page. - Keep the age mark field to compute AllocatedSinceLastGC accurately. TODO: Adjust tests to new expectations. BUG=chromium:581412 ========== to ========== [heap] Remove border page - Age is now specified solely by the page itself. - Upon setting an age mark, we create a filler so that subsequent allocations target the next page. - Keep the age mark field to compute AllocatedSinceLastGC accurately. TODO: Adjust tests to new expectations. BUG=chromium:581412 ==========
Patchset #4 (id:60001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== [heap] Remove border page - Age is now specified solely by the page itself. - Upon setting an age mark, we create a filler so that subsequent allocations target the next page. - Keep the age mark field to compute AllocatedSinceLastGC accurately. TODO: Adjust tests to new expectations. BUG=chromium:581412 ========== to ========== [heap] Remove border page A age now belongs either the nursery *or* the intermediate space. The page that contained objects of both spaces is removed in this change. More specifically: - Age is determined solely by NEW_SPACE_BELOW_AGE_MARK flag. - Upon setting an age mark, we create a filler so that subsequent allocations target the next page. TODO: Adjust tests to new expectations. BUG=chromium:581412 ==========
Description was changed from ========== [heap] Remove border page A age now belongs either the nursery *or* the intermediate space. The page that contained objects of both spaces is removed in this change. More specifically: - Age is determined solely by NEW_SPACE_BELOW_AGE_MARK flag. - Upon setting an age mark, we create a filler so that subsequent allocations target the next page. TODO: Adjust tests to new expectations. BUG=chromium:581412 ========== to ========== [heap] Remove border page A page now belongs either the nursery *or* the intermediate space. The page that contained objects of both spaces is removed in this change. More specifically: - Age is determined solely by NEW_SPACE_BELOW_AGE_MARK flag. - Upon setting an age mark, we create a filler so that subsequent allocations target the next page. TODO: Adjust tests to new expectations. BUG=chromium:581412 ==========
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/6733) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
Description was changed from ========== [heap] Remove border page A page now belongs either the nursery *or* the intermediate space. The page that contained objects of both spaces is removed in this change. More specifically: - Age is determined solely by NEW_SPACE_BELOW_AGE_MARK flag. - Upon setting an age mark, we create a filler so that subsequent allocations target the next page. TODO: Adjust tests to new expectations. BUG=chromium:581412 ========== to ========== [heap] Remove border page A page now belongs either the nursery *or* the intermediate space. The page that contained objects of both spaces is removed in this change. More specifically: - Age is determined solely by NEW_SPACE_BELOW_AGE_MARK flag. - Upon setting an age mark, we create a filler so that subsequent allocations target the next page. BUG=chromium:581412 ==========
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/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [heap] Remove border page A page now belongs either the nursery *or* the intermediate space. The page that contained objects of both spaces is removed in this change. More specifically: - Age is determined solely by NEW_SPACE_BELOW_AGE_MARK flag. - Upon setting an age mark, we create a filler so that subsequent allocations target the next page. BUG=chromium:581412 ========== to ========== [heap] Remove border page A page now belongs either the nursery *or* the intermediate space. The page that contained objects of both spaces is removed in this change. More specifically: - Age is determined solely by NEW_SPACE_BELOW_AGE_MARK flag. - Upon setting an age mark, we create a filler so that subsequent allocations target the next page. BUG=chromium:chromium:636331 ==========
Description was changed from ========== [heap] Remove border page A page now belongs either the nursery *or* the intermediate space. The page that contained objects of both spaces is removed in this change. More specifically: - Age is determined solely by NEW_SPACE_BELOW_AGE_MARK flag. - Upon setting an age mark, we create a filler so that subsequent allocations target the next page. BUG=chromium:chromium:636331 ========== to ========== [heap] Remove border page A page now belongs either the nursery *or* the intermediate space. The page that contained objects of both spaces is removed in this change. More specifically: - Age is determined solely by NEW_SPACE_BELOW_AGE_MARK flag. - Upon setting an age mark, we create a filler so that subsequent allocations target the next page. BUG=chromium:636331 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_arm_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...)
Patchset #7 (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/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [heap] Remove border page A page now belongs either the nursery *or* the intermediate space. The page that contained objects of both spaces is removed in this change. More specifically: - Age is determined solely by NEW_SPACE_BELOW_AGE_MARK flag. - Upon setting an age mark, we create a filler so that subsequent allocations target the next page. BUG=chromium:636331 ========== to ========== [heap] Remove border page A page now belongs either the nursery *or* the intermediate space. The page that contained objects of both spaces is removed in this change. Age is determined solely by IN_INTERMEDIATE_GENERATION flag. BUG=chromium:636331 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/15022) v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/9638) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
Patchset #9 (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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [heap] Remove border page A page now belongs either the nursery *or* the intermediate space. The page that contained objects of both spaces is removed in this change. Age is determined solely by IN_INTERMEDIATE_GENERATION flag. BUG=chromium:636331 ========== to ========== [heap] Remove border page A page now belongs either the nursery *or* the intermediate space. The page that contained objects of both spaces is removed in this change. BUG=chromium:636331 ==========
mlippautz@chromium.org changed reviewers: + hpayer@chromium.org, ulan@chromium.org
PTAL Will check benchmarks for fragmentation issues introduced by sealing the intermediate generation.
Description was changed from ========== [heap] Remove border page A page now belongs either the nursery *or* the intermediate space. The page that contained objects of both spaces is removed in this change. BUG=chromium:636331 ========== to ========== [heap] Remove border page A page now belongs either the nursery *or* the intermediate gen. The page that contained objects of both spaces is removed in this change. BUG=chromium:636331 ==========
https://codereview.chromium.org/2209583002/diff/240001/src/heap/spaces-inl.h File src/heap/spaces-inl.h (right): https://codereview.chromium.org/2209583002/diff/240001/src/heap/spaces-inl.h#... src/heap/spaces-inl.h:179: : Page::kAllocatableMemory; This method is not precise. Fragmentation at the end of a page is counted as allocated. https://codereview.chromium.org/2209583002/diff/240001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/2209583002/diff/240001/src/heap/spaces.cc#new... src/heap/spaces.cc:1598: new_space->SetAllocationInfo(page_high(), page_high()); can we set top and limit to null? https://codereview.chromium.org/2209583002/diff/240001/src/heap/spaces.cc#new... src/heap/spaces.cc:2105: allocation_info_.Reset(to_space_.page_high(), to_space_.page_high()); same as above, wouldn't it be cleaner to set top and limit to null here? https://codereview.chromium.org/2209583002/diff/240001/test/cctest/heap/test-... File test/cctest/heap/test-heap.cc (right): https://codereview.chromium.org/2209583002/diff/240001/test/cctest/heap/test-... test/cctest/heap/test-heap.cc:2365: v8::HandleScope scope(CcTest::isolate()); Why?
Added the parts that allow an empty intermediate generation (+test). https://codereview.chromium.org/2209583002/diff/240001/src/heap/spaces-inl.h File src/heap/spaces-inl.h (right): https://codereview.chromium.org/2209583002/diff/240001/src/heap/spaces-inl.h#... src/heap/spaces-inl.h:179: : Page::kAllocatableMemory; On 2016/09/27 08:28:31, Hannes Payer wrote: > This method is not precise. Fragmentation at the end of a page is counted as > allocated. Ack. Let's try changing this in a different CL as it affects heuristics. https://codereview.chromium.org/2209583002/diff/240001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/2209583002/diff/240001/src/heap/spaces.cc#new... src/heap/spaces.cc:1598: new_space->SetAllocationInfo(page_high(), page_high()); On 2016/09/27 08:28:31, Hannes Payer wrote: > can we set top and limit to null? As discussed offline, there are quite some callers that expect top() to be non-null (e.g. accounting, iterating page ranges, ...). https://codereview.chromium.org/2209583002/diff/240001/src/heap/spaces.cc#new... src/heap/spaces.cc:2105: allocation_info_.Reset(to_space_.page_high(), to_space_.page_high()); On 2016/09/27 08:28:31, Hannes Payer wrote: > same as above, wouldn't it be cleaner to set top and limit to null here? See above. https://codereview.chromium.org/2209583002/diff/240001/test/cctest/heap/test-... File test/cctest/heap/test-heap.cc (right): https://codereview.chromium.org/2209583002/diff/240001/test/cctest/heap/test-... test/cctest/heap/test-heap.cc:2365: v8::HandleScope scope(CcTest::isolate()); On 2016/09/27 08:28:32, Hannes Payer wrote: > Why? Left-over from fixing the test. Removed.
cool, 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/v2/patch-status/codereview.chromium.or...
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/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [heap] Remove border page A page now belongs either the nursery *or* the intermediate gen. The page that contained objects of both spaces is removed in this change. BUG=chromium:636331 ========== to ========== [heap] Remove border page A page now belongs either the nursery *or* the intermediate gen. The page that contained objects of both spaces is removed in this change. BUG=chromium:636331 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== [heap] Remove border page A page now belongs either the nursery *or* the intermediate gen. The page that contained objects of both spaces is removed in this change. BUG=chromium:636331 ========== to ========== [heap] Remove border page A page now belongs either the nursery *or* the intermediate gen. The page that contained objects of both spaces is removed in this change. BUG=chromium:636331 Committed: https://crrev.com/42ece47446f0dbd3779d6e0e00dce97a1931a9f9 Cr-Commit-Position: refs/heads/master@{#39778} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/42ece47446f0dbd3779d6e0e00dce97a1931a9f9 Cr-Commit-Position: refs/heads/master@{#39778}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:260001) has been created in https://codereview.chromium.org/2374253003/ by mlippautz@chromium.org. The reason for reverting is: No real improvement as we still lack the ability to promote from scavenges/young gen GCs. Let's keep this in mind for later.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
