improve allocation accounting for incremental mark

improve allocation accounting for incremental mark

Add an assertion that allocated_bytes >= 0 in IncrementalMark::Step and then
make it pass. We were not being diligent in maintaining top_on_previous_step_
and as a result inaccurate, and even negative values of allocated_bytes were
being reported to Step.

BUG=
R=hpayer@chromium.org

Committed: https://crrev.com/007baaedd30fb506569b5f649eddd456197eecae
Cr-Commit-Position: refs/heads/master@{#30778}

[ofrobots, 2015-08-04 19:37:11]

https://codereview.chromium.org/1274453002/diff/1/src/heap/spaces.cc
File src/heap/spaces.cc (right):

https://codereview.chromium.org/1274453002/diff/1/src/heap/spaces.cc#newcode1517
src/heap/spaces.cc:1517: top_on_previous_step_ = new_top;
I think it might be good to refactor this into a new
NewSpace::InlineAllocationStep method. I can do that. I wanted to keep the
initial CL minimal.

[ofrobots, 2015-08-14 02:36:51]

On 2015/08/04 19:37:11, ofrobots wrote:
> https://codereview.chromium.org/1274453002/diff/1/src/heap/spaces.cc
> File src/heap/spaces.cc (right):
> 
>
https://codereview.chromium.org/1274453002/diff/1/src/heap/spaces.cc#newcode1517
> src/heap/spaces.cc:1517: top_on_previous_step_ = new_top;
> I think it might be good to refactor this into a new
> NewSpace::InlineAllocationStep method. I can do that. I wanted to keep the
> initial CL minimal.

Ping hpayer@. 

PTAL.

[Hannes Payer (out of office), 2015-08-14 05:26:30]

https://codereview.chromium.org/1274453002/diff/1/src/heap/spaces.cc
File src/heap/spaces.cc (right):

https://codereview.chromium.org/1274453002/diff/1/src/heap/spaces.cc#newcode1410
src/heap/spaces.cc:1410: heap()->incremental_marking()->Step(bytes_allocated,
Calling step is is a no-op in this case. ResetAllocationInfo is always called
within the GC. if (heap_->gc_state() != Heap::NOT_IN_GC ... will bail out
immediately.

https://codereview.chromium.org/1274453002/diff/1/src/heap/spaces.cc#newcode1517
src/heap/spaces.cc:1517: top_on_previous_step_ = new_top;
On 2015/08/04 19:37:11, ofrobots wrote:
> I think it might be good to refactor this into a new
> NewSpace::InlineAllocationStep method. I can do that. I wanted to keep the
> initial CL minimal.

Yes, good idea.

[ofrobots, 2015-08-14 15:26:18]

https://codereview.chromium.org/1274453002/diff/1/src/heap/spaces.cc
File src/heap/spaces.cc (right):

https://codereview.chromium.org/1274453002/diff/1/src/heap/spaces.cc#newcode1410
src/heap/spaces.cc:1410: heap()->incremental_marking()->Step(bytes_allocated,
On 2015/08/14 05:26:30, Hannes Payer wrote:
> Calling step is is a no-op in this case. ResetAllocationInfo is always called
> within the GC. if (heap_->gc_state() != Heap::NOT_IN_GC ... will bail out
> immediately.

The objective is to make accounting of the allocated bytes accurate. Today,
incremental_marking doesn't care about the bytes allocated during GC, but there
might be other consumers of the allocated bytes (such as a sampling heap
profiler) that do care about these bytes. It does make sense to do a
inline-allocation-step here, even if incremental_marking is going to ignore
them.

Once I refactor this into NewSpace::InlineAllocationStep, this would become more
obvious.

[Hannes Payer (out of office), 2015-08-17 17:56:16]

lgtm

https://codereview.chromium.org/1274453002/diff/1/src/heap/spaces.cc
File src/heap/spaces.cc (right):

https://codereview.chromium.org/1274453002/diff/1/src/heap/spaces.cc#newcode1410
src/heap/spaces.cc:1410: heap()->incremental_marking()->Step(bytes_allocated,
On 2015/08/14 15:26:18, ofrobots wrote:
> On 2015/08/14 05:26:30, Hannes Payer wrote:
> > Calling step is is a no-op in this case. ResetAllocationInfo is always
called
> > within the GC. if (heap_->gc_state() != Heap::NOT_IN_GC ... will bail out
> > immediately.
> 
> The objective is to make accounting of the allocated bytes accurate. Today,
> incremental_marking doesn't care about the bytes allocated during GC, but
there
> might be other consumers of the allocated bytes (such as a sampling heap
> profiler) that do care about these bytes. It does make sense to do a
> inline-allocation-step here, even if incremental_marking is going to ignore
> them.
> 
> Once I refactor this into NewSpace::InlineAllocationStep, this would become
more
> obvious.

SGTM with the refactoring.

[ofrobots, 2015-09-17 00:10:44]

The CQ bit was checked by ofrobots@google.com

[commit-bot: I haz the power, 2015-09-17 00:10:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1274453002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1274453002/1

[commit-bot: I haz the power, 2015-09-17 00:55:30]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-09-17 00:55:42]

Patchset 1 (id:??) landed as
https://crrev.com/007baaedd30fb506569b5f649eddd456197eecae
Cr-Commit-Position: refs/heads/master@{#30778}

[ofrobots, 2015-09-17 20:06:44]

On 2015/08/17 17:56:16, Hannes Payer wrote:
> lgtm
> 
> https://codereview.chromium.org/1274453002/diff/1/src/heap/spaces.cc
> File src/heap/spaces.cc (right):
> 
>
https://codereview.chromium.org/1274453002/diff/1/src/heap/spaces.cc#newcode1410
> src/heap/spaces.cc:1410: heap()->incremental_marking()->Step(bytes_allocated,
> On 2015/08/14 15:26:18, ofrobots wrote:
> > On 2015/08/14 05:26:30, Hannes Payer wrote:
> > > Calling step is is a no-op in this case. ResetAllocationInfo is always
> called
> > > within the GC. if (heap_->gc_state() != Heap::NOT_IN_GC ... will bail out
> > > immediately.
> > 
> > The objective is to make accounting of the allocated bytes accurate. Today,
> > incremental_marking doesn't care about the bytes allocated during GC, but
> there
> > might be other consumers of the allocated bytes (such as a sampling heap
> > profiler) that do care about these bytes. It does make sense to do a
> > inline-allocation-step here, even if incremental_marking is going to ignore
> > them.
> > 
> > Once I refactor this into NewSpace::InlineAllocationStep, this would become
> more
> > obvious.
> 
> SGTM with the refactoring.

Refactoring in https://codereview.chromium.org/1351983002.