Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1511933002:
[cctest] Add tests for aborting compaction of pages (Closed)
|
Created:
5 years ago by Michael Lippautz Modified:
5 years 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[cctest] Add tests for aborting compaction of pages
Tests for
* aborting a full page.
* partially aborting a page.
* partially aborting a page with pointers between aborted pages.
* partially aborting a page with store buffer entries.
Also introduces force_oom() which prohibits a old space to
expand
BUG=chromium:524425
LOG=N
Patch Set 1 #Patch Set 2 : More tests #
Total comments: 4
Patch Set 3 : Addressed comments; fixed typos #
Total comments: 2
Patch Set 4 : Addressed comment #
Total comments: 2
Patch Set 5 : Addressed final comment #Patch Set 6 : Workaround FLAG_concurrent_sweeping not being respected after heap initialization #
Messages
Total messages: 36 (20 generated)
Description was changed from ========== [cctest] Add tests for aborting compaction BUG= ========== to ========== [cctest] Add tests for aborting compaction BUG=chromium:524425 LOG=N ==========
Description was changed from ========== [cctest] Add tests for aborting compaction BUG=chromium:524425 LOG=N ========== to ========== [cctest] Add tests for aborting compaction of a single page * Also introduces ShouldForceOOM() which prohibits a PagedSpace from expanding. Compaction spaces refer to the corresponding actual space. BUG=chromium:524425 LOG=N ==========
Description was changed from ========== [cctest] Add tests for aborting compaction of a single page * Also introduces ShouldForceOOM() which prohibits a PagedSpace from expanding. Compaction spaces refer to the corresponding actual space. BUG=chromium:524425 LOG=N ========== to ========== [cctest] Add tests for aborting compaction of a single page * Also introduces ShouldForceOOM() which prohibits a PagedSpace from expanding. Compaction spaces refer to the corresponding actual space. BUG=chromium:524425 LOG=N ==========
Description was changed from ========== [cctest] Add tests for aborting compaction of a single page * Also introduces ShouldForceOOM() which prohibits a PagedSpace from expanding. Compaction spaces refer to the corresponding actual space. BUG=chromium:524425 LOG=N ========== to ========== [cctest] Add tests for aborting compaction of a single page Tests for * aborting a full page. * partially aborting a page. * partially aborting a page with pointers between aborted pages. * partially aborting a page with store buffer entries. Also introduces ShouldForceOOM() which prohibits a PagedSpace from expanding. Compaction spaces refer to the corresponding actual space. BUG=chromium:524425 LOG=N ==========
Description was changed from ========== [cctest] Add tests for aborting compaction of a single page Tests for * aborting a full page. * partially aborting a page. * partially aborting a page with pointers between aborted pages. * partially aborting a page with store buffer entries. Also introduces ShouldForceOOM() which prohibits a PagedSpace from expanding. Compaction spaces refer to the corresponding actual space. BUG=chromium:524425 LOG=N ========== to ========== [cctest] Add tests for aborting compaction of a single page Tests for * aborting a full page. * partially aborting a page. * partially aborting a page with pointers between aborted pages. * partially aborting a page with store buffer entries. Also introduces ShouldForceOOM() which prohibits a PagedSpace from expanding. Compaction spaces refer to the corresponding actual space. BUG=chromium:524425 LOG=N ==========
Patchset #2 (id:20001) has been deleted
Description was changed from ========== [cctest] Add tests for aborting compaction of a single page Tests for * aborting a full page. * partially aborting a page. * partially aborting a page with pointers between aborted pages. * partially aborting a page with store buffer entries. Also introduces ShouldForceOOM() which prohibits a PagedSpace from expanding. Compaction spaces refer to the corresponding actual space. BUG=chromium:524425 LOG=N ========== to ========== [cctest] Add tests for aborting compaction of pages Tests for * aborting a full page. * partially aborting a page. * partially aborting a page with pointers between aborted pages. * partially aborting a page with store buffer entries. Also introduces ShouldForceOOM() which prohibits a PagedSpace from expanding. Compaction spaces refer to the corresponding actual space. BUG=chromium:524425 LOG=N ==========
Description was changed from ========== [cctest] Add tests for aborting compaction of pages Tests for * aborting a full page. * partially aborting a page. * partially aborting a page with pointers between aborted pages. * partially aborting a page with store buffer entries. Also introduces ShouldForceOOM() which prohibits a PagedSpace from expanding. Compaction spaces refer to the corresponding actual space. BUG=chromium:524425 LOG=N ========== to ========== [cctest] Add tests for aborting compaction of pages Tests for * aborting a full page. * partially aborting a page. * partially aborting a page with pointers between aborted pages. * partially aborting a page with store buffer entries. Also introduces ShouldForceOOM() which prohibits a PagedSpace from expanding. Compaction spaces refer to the corresponding actual space. BUG=chromium:524425 LOG=N ==========
mlippautz@chromium.org changed reviewers: + hpayer@chromium.org, ulan@chromium.org
PTAL. I am open to different ideas of simulating the OOM scenario. In general, the problem we currently have when writing these kind of tests is the lack of ability to force allocation of an initialized object (e.g. FixedArray, String) on a specific page. Also, the concurrent sweeper makes things non-deterministic, since we cannot be sure how much memory can already be used for compaction.
https://codereview.chromium.org/1511933002/diff/40001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1511933002/diff/40001/src/heap/spaces.cc#newc... src/heap/spaces.cc:1185: if (!heap()->CanExpandOldGeneration(static_cast<int>(size))) return false; I think that ShouldForeOOM should be a property of heap and CanExpandOldGeneration should check that.
lgtm modulo Hannes' comments https://codereview.chromium.org/1511933002/diff/40001/test/cctest/heap/test-c... File test/cctest/heap/test-compaction.cc (right): https://codereview.chromium.org/1511933002/diff/40001/test/cctest/heap/test-c... test/cctest/heap/test-compaction.cc:122: } else CHECK(the object is on the third page) ?
Patchset #3 (id:60001) has been deleted
Addressed comments and fixed typos in the descriptive comments in the tests. https://codereview.chromium.org/1511933002/diff/40001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1511933002/diff/40001/src/heap/spaces.cc#newc... src/heap/spaces.cc:1185: if (!heap()->CanExpandOldGeneration(static_cast<int>(size))) return false; On 2015/12/10 15:18:14, Hannes Payer wrote: > I think that ShouldForeOOM should be a property of heap and > CanExpandOldGeneration should check that. Done. It's still called force_oom(). https://codereview.chromium.org/1511933002/diff/40001/test/cctest/heap/test-c... File test/cctest/heap/test-compaction.cc (right): https://codereview.chromium.org/1511933002/diff/40001/test/cctest/heap/test-c... test/cctest/heap/test-compaction.cc:122: } On 2015/12/10 17:29:23, ulan wrote: > else CHECK(the object is on the third page) ? Done.
https://codereview.chromium.org/1511933002/diff/80001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1511933002/diff/80001/src/heap/spaces.cc#newc... src/heap/spaces.cc:1171: if (heap()->force_oom()) return false; This check should be done within heap()->CanExpandOldGeneration Then you can also remove the friend.
https://codereview.chromium.org/1511933002/diff/80001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1511933002/diff/80001/src/heap/spaces.cc#newc... src/heap/spaces.cc:1171: if (heap()->force_oom()) return false; On 2015/12/10 18:11:54, Hannes Payer wrote: > This check should be done within heap()->CanExpandOldGeneration > > Then you can also remove the friend. Done. Thanks for clarifying!
LGTM, one nit https://codereview.chromium.org/1511933002/diff/100001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1511933002/diff/100001/src/heap/heap.h#newcod... src/heap/heap.h:2118: bool force_oom() { return force_oom_; } you can remove the getter since it is just used internally.
https://codereview.chromium.org/1511933002/diff/100001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1511933002/diff/100001/src/heap/heap.h#newcod... src/heap/heap.h:2118: bool force_oom() { return force_oom_; } On 2015/12/10 18:32:06, Hannes Payer wrote: > you can remove the getter since it is just used internally. Done.
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, hpayer@chromium.org Link to the patchset: https://codereview.chromium.org/1511933002/#ps120001 (title: "Addressed final comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1511933002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1511933002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_win_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/402) v8_win_rel_ng_triggered on 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/patch-status/1511933002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1511933002/140001
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, ulan@chromium.org Link to the patchset: https://codereview.chromium.org/1511933002/#ps140001 (title: "Workaround FLAG_concurrent_sweeping not being respected after heap initialization")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1511933002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1511933002/140001
Message was sent while issue was closed.
Description was changed from ========== [cctest] Add tests for aborting compaction of pages Tests for * aborting a full page. * partially aborting a page. * partially aborting a page with pointers between aborted pages. * partially aborting a page with store buffer entries. Also introduces ShouldForceOOM() which prohibits a PagedSpace from expanding. Compaction spaces refer to the corresponding actual space. BUG=chromium:524425 LOG=N ========== to ========== [cctest] Add tests for aborting compaction of pages Tests for * aborting a full page. * partially aborting a page. * partially aborting a page with pointers between aborted pages. * partially aborting a page with store buffer entries. Also introduces ShouldForceOOM() which prohibits a PagedSpace from expanding. Compaction spaces refer to the corresponding actual space. BUG=chromium:524425 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [cctest] Add tests for aborting compaction of pages Tests for * aborting a full page. * partially aborting a page. * partially aborting a page with pointers between aborted pages. * partially aborting a page with store buffer entries. Also introduces ShouldForceOOM() which prohibits a PagedSpace from expanding. Compaction spaces refer to the corresponding actual space. BUG=chromium:524425 LOG=N ========== to ========== [cctest] Add tests for aborting compaction of pages Tests for * aborting a full page. * partially aborting a page. * partially aborting a page with pointers between aborted pages. * partially aborting a page with store buffer entries. Also introduces ShouldForceOOM() which prohibits a PagedSpace from expanding. Compaction spaces refer to the corresponding actual space. BUG=chromium:524425 LOG=N Committed: https://crrev.com/161a0e00519a63672d93c36c5d3854c8ccb0030f Cr-Commit-Position: refs/heads/master@{#32783} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/161a0e00519a63672d93c36c5d3854c8ccb0030f Cr-Commit-Position: refs/heads/master@{#32783}
Message was sent while issue was closed.
Description was changed from ========== [cctest] Add tests for aborting compaction of pages Tests for * aborting a full page. * partially aborting a page. * partially aborting a page with pointers between aborted pages. * partially aborting a page with store buffer entries. Also introduces ShouldForceOOM() which prohibits a PagedSpace from expanding. Compaction spaces refer to the corresponding actual space. BUG=chromium:524425 LOG=N Committed: https://crrev.com/161a0e00519a63672d93c36c5d3854c8ccb0030f Cr-Commit-Position: refs/heads/master@{#32783} ========== to ========== [cctest] Add tests for aborting compaction of pages Tests for * aborting a full page. * partially aborting a page. * partially aborting a page with pointers between aborted pages. * partially aborting a page with store buffer entries. Also introduces force_oom() which prohibits a old space to expand BUG=chromium:524425 LOG=N ==========
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:140001) has been created in https://codereview.chromium.org/1514603008/ by mlippautz@chromium.org. The reason for reverting is: Failing on Win 32bit nosnap: https://chromegw.corp.google.com/i/client.v8/builders/V8%20Win32%20-%20nosnap.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
