[heap] Handle edge case in NewSpaceAllocationCounter test

[heap] Handle edge case in NewSpaceAllocationCounter test

This test checks that counters accurately reflect the allocated size.
There's an edge case that can occur when, previously to the allocation,
the page does not have enough space left to allocate the requested
object - then we move on to a fresh page, fill the remaining space of
the old page with a filler object, and allocate the requested object on
the new page.

The counters will show the size of the filler object plus the requested
object size, while the test expects only the requested size.

This CL fixes that case by performing two GCs to clear out new space.

BUG=

Review-Url: https://codereview.chromium.org/2652933002
Cr-Commit-Position: refs/heads/master@{#42646}
Committed: https://chromium.googlesource.com/v8/v8/+/4ec372801e2e4c26659d046ecae8f2661b303487

[jgruber, 2017-01-24 09:56:35]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 09:56:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jgruber, 2017-01-24 10:03:53]

jgruber@chromium.org changed reviewers:
+ mlippautz@chromium.org

[jgruber, 2017-01-24 10:03:53]

This fixes the test failure at [0], triggered by [1] on arm64 nosnap debug
builds. Not sure if this fix is 100% robust (since after EnsureSpace we trigger
a GC). Please take a look.

[0]
https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%....
[1] https://codereview.chromium.org/2630233003/

[commit-bot: I haz the power, 2017-01-24 10:32:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 10:32:15]

Dry run: This issue passed the CQ dry run.

[Michael Lippautz, 2017-01-24 13:53:28]

On 2017/01/24 10:03:53, jgruber wrote:
> This fixes the test failure at [0], triggered by [1] on arm64 nosnap debug
> builds. Not sure if this fix is 100% robust (since after EnsureSpace we
trigger
> a GC). Please take a look.
> 
> [0]
>
https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%....
> [1] https://codereview.chromium.org/2630233003/

Could you try replacing the EnsureAllocation with 2 NEW_SPACE GCs? The new space
should be empty then and we should always be able to allocate the 1024kb, making
this test completely robust. 

If that doesn't work just let me know and we will go with the approach proposed
here.

[jgruber, 2017-01-24 15:31:54]

On 2017/01/24 13:53:28, Michael Lippautz wrote:
> On 2017/01/24 10:03:53, jgruber wrote:
> > This fixes the test failure at [0], triggered by [1] on arm64 nosnap debug
> > builds. Not sure if this fix is 100% robust (since after EnsureSpace we
> trigger
> > a GC). Please take a look.
> > 
> > [0]
> >
>
https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%....
> > [1] https://codereview.chromium.org/2630233003/
> 
> Could you try replacing the EnsureAllocation with 2 NEW_SPACE GCs? The new
space
> should be empty then and we should always be able to allocate the 1024kb,
making
> this test completely robust. 
> 
> If that doesn't work just let me know and we will go with the approach
proposed
> here.

Unfortunately the test continues to fail with your suggestion :( Used patch:

--- a/test/cctest/heap/test-heap.cc
+++ b/test/cctest/heap/test-heap.cc
@@ -6164,6 +6164,8 @@ TEST(NewSpaceAllocationCounter) {
   size_t counter2 = heap->NewSpaceAllocationCounter();
   CHECK_EQ(kSize, counter2 - counter1);
   CcTest::CollectGarbage(NEW_SPACE);
+  CcTest::CollectGarbage(NEW_SPACE);
+  CcTest::CollectGarbage(NEW_SPACE);  // Once more to empty new space.
   size_t counter3 = heap->NewSpaceAllocationCounter();
   CHECK_EQ(0U, counter3 - counter2);
   // Test counter overflow.


For posterity, repro instructions are:

$ git checkout 70000946eb2a9155679528702a766219a1fcf154
$ ninja -j512 -l12 -C out/debug-nosnapshot/ cctest
$ out/debug-nosnapshot/cctest --random-seed=-976563914
test-heap/NewSpaceAllocationCounter --nohard-abort --nodead-code-elimination
--nofold-constants --enable-slow-asserts --verify-heap

#
# Fatal error in ../../test/cctest/heap/test-heap.cc, line 6165
# Check failed: kSize == counter2 - counter1 (1024 vs. 1872).
#

[jgruber, 2017-01-24 15:46:29]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 15:46:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jgruber, 2017-01-24 15:47:16]

On 2017/01/24 15:31:54, jgruber wrote:
> On 2017/01/24 13:53:28, Michael Lippautz wrote:
> > On 2017/01/24 10:03:53, jgruber wrote:
> > > This fixes the test failure at [0], triggered by [1] on arm64 nosnap debug
> > > builds. Not sure if this fix is 100% robust (since after EnsureSpace we
> > trigger
> > > a GC). Please take a look.
> > > 
> > > [0]
> > >
> >
>
https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%....
> > > [1] https://codereview.chromium.org/2630233003/
> > 
> > Could you try replacing the EnsureAllocation with 2 NEW_SPACE GCs? The new
> space
> > should be empty then and we should always be able to allocate the 1024kb,
> making
> > this test completely robust. 
> > 
> > If that doesn't work just let me know and we will go with the approach
> proposed
> > here.
> 
> Unfortunately the test continues to fail with your suggestion :( Used patch:
> 
> --- a/test/cctest/heap/test-heap.cc
> +++ b/test/cctest/heap/test-heap.cc
> @@ -6164,6 +6164,8 @@ TEST(NewSpaceAllocationCounter) {
>    size_t counter2 = heap->NewSpaceAllocationCounter();
>    CHECK_EQ(kSize, counter2 - counter1);
>    CcTest::CollectGarbage(NEW_SPACE);
> +  CcTest::CollectGarbage(NEW_SPACE);
> +  CcTest::CollectGarbage(NEW_SPACE);  // Once more to empty new space.
>    size_t counter3 = heap->NewSpaceAllocationCounter();
>    CHECK_EQ(0U, counter3 - counter2);
>    // Test counter overflow.
> 
> 
> For posterity, repro instructions are:
> 
> $ git checkout 70000946eb2a9155679528702a766219a1fcf154
> $ ninja -j512 -l12 -C out/debug-nosnapshot/ cctest
> $ out/debug-nosnapshot/cctest --random-seed=-976563914
> test-heap/NewSpaceAllocationCounter --nohard-abort --nodead-code-elimination
> --nofold-constants --enable-slow-asserts --verify-heap
> 
> #
> # Fatal error in ../../test/cctest/heap/test-heap.cc, line 6165
> # Check failed: kSize == counter2 - counter1 (1024 vs. 1872).
> #

Hmm, it works now, I probably messed up the build. Michi, the new patchset
uses your suggestion.

[jgruber, 2017-01-24 15:47:42]

Description was changed from

==========
[heap] Handle edge case in NewSpaceAllocationCounter test

This test checks that counters accurately reflect the allocated size.
There's an edge case that can occur when, previously to the allocation,
the page does not have enough space left to allocate the requested
object - then we move on to a fresh page, fill the remaining space of
the old page with a filler object, and allocate the requested object on
the new page.

The counters will show the size of the filler object plus the requested
object size, while the test expects only the requested size.

This CL fixes that case by ensuring that the current page has sufficient
space.

BUG=
==========

to

==========
[heap] Handle edge case in NewSpaceAllocationCounter test

This test checks that counters accurately reflect the allocated size.
There's an edge case that can occur when, previously to the allocation,
the page does not have enough space left to allocate the requested
object - then we move on to a fresh page, fill the remaining space of
the old page with a filler object, and allocate the requested object on
the new page.

The counters will show the size of the filler object plus the requested
object size, while the test expects only the requested size.

This CL fixes that case by performing two GCs to clear out new space.

BUG=
==========

[Michael Lippautz, 2017-01-24 15:49:44]

On 2017/01/24 15:47:16, jgruber wrote:
> On 2017/01/24 15:31:54, jgruber wrote:
> > On 2017/01/24 13:53:28, Michael Lippautz wrote:
> > > On 2017/01/24 10:03:53, jgruber wrote:
> > > > This fixes the test failure at [0], triggered by [1] on arm64 nosnap
debug
> > > > builds. Not sure if this fix is 100% robust (since after EnsureSpace we
> > > trigger
> > > > a GC). Please take a look.
> > > > 
> > > > [0]
> > > >
> > >
> >
>
https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%....
> > > > [1] https://codereview.chromium.org/2630233003/
> > > 
> > > Could you try replacing the EnsureAllocation with 2 NEW_SPACE GCs? The new
> > space
> > > should be empty then and we should always be able to allocate the 1024kb,
> > making
> > > this test completely robust. 
> > > 
> > > If that doesn't work just let me know and we will go with the approach
> > proposed
> > > here.
> > 
> > Unfortunately the test continues to fail with your suggestion :( Used patch:
> > 
> > --- a/test/cctest/heap/test-heap.cc
> > +++ b/test/cctest/heap/test-heap.cc
> > @@ -6164,6 +6164,8 @@ TEST(NewSpaceAllocationCounter) {
> >    size_t counter2 = heap->NewSpaceAllocationCounter();
> >    CHECK_EQ(kSize, counter2 - counter1);
> >    CcTest::CollectGarbage(NEW_SPACE);
> > +  CcTest::CollectGarbage(NEW_SPACE);
> > +  CcTest::CollectGarbage(NEW_SPACE);  // Once more to empty new space.
> >    size_t counter3 = heap->NewSpaceAllocationCounter();
> >    CHECK_EQ(0U, counter3 - counter2);
> >    // Test counter overflow.
> > 
> > 
> > For posterity, repro instructions are:
> > 
> > $ git checkout 70000946eb2a9155679528702a766219a1fcf154
> > $ ninja -j512 -l12 -C out/debug-nosnapshot/ cctest
> > $ out/debug-nosnapshot/cctest --random-seed=-976563914
> > test-heap/NewSpaceAllocationCounter --nohard-abort --nodead-code-elimination
> > --nofold-constants --enable-slow-asserts --verify-heap
> > 
> > #
> > # Fatal error in ../../test/cctest/heap/test-heap.cc, line 6165
> > # Check failed: kSize == counter2 - counter1 (1024 vs. 1872).
> > #
> 
> Hmm, it works now, I probably messed up the build. Michi, the new patchset
> uses your suggestion.

lgtm

[jgruber, 2017-01-24 15:53:00]

The CQ bit was unchecked by jgruber@chromium.org

[jgruber, 2017-01-24 15:53:06]

The CQ bit was checked by jgruber@chromium.org

[commit-bot: I haz the power, 2017-01-24 15:53:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 15:57:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 15:57:50]

Try jobs failed on following builders:
  v8_linux_chromium_gn_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)

[jgruber, 2017-01-25 07:36:54]

The CQ bit was checked by jgruber@chromium.org

[commit-bot: I haz the power, 2017-01-25 07:37:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 07:41:35]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485329814438300,
"parent_rev": "39b858754e0b6d9a75e5525e50e9bf378b6c9472", "commit_rev":
"4ec372801e2e4c26659d046ecae8f2661b303487"}

[commit-bot: I haz the power, 2017-01-25 07:41:38]

Description was changed from

==========
[heap] Handle edge case in NewSpaceAllocationCounter test

This test checks that counters accurately reflect the allocated size.
There's an edge case that can occur when, previously to the allocation,
the page does not have enough space left to allocate the requested
object - then we move on to a fresh page, fill the remaining space of
the old page with a filler object, and allocate the requested object on
the new page.

The counters will show the size of the filler object plus the requested
object size, while the test expects only the requested size.

This CL fixes that case by performing two GCs to clear out new space.

BUG=
==========

to

==========
[heap] Handle edge case in NewSpaceAllocationCounter test

This test checks that counters accurately reflect the allocated size.
There's an edge case that can occur when, previously to the allocation,
the page does not have enough space left to allocate the requested
object - then we move on to a fresh page, fill the remaining space of
the old page with a filler object, and allocate the requested object on
the new page.

The counters will show the size of the filler object plus the requested
object size, while the test expects only the requested size.

This CL fixes that case by performing two GCs to clear out new space.

BUG=

Review-Url: https://codereview.chromium.org/2652933002
Cr-Commit-Position: refs/heads/master@{#42646}
Committed:
https://chromium.googlesource.com/v8/v8/+/4ec372801e2e4c26659d046ecae8f2661b3...
==========

[commit-bot: I haz the power, 2017-01-25 07:41:39]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/4ec372801e2e4c26659d046ecae8f2661b3...