[heap] inline allocation steps refactor

[heap] inline allocation steps refactor

Expose the steps for incremental marking and idle scavenge more directly in
NewSpace. Adjust the NewSpace and Heap interfaces to allow callers to be more
clear about how they are interacting with inline allocation steps. This refactor
prepares the ground for more consumers of inline allocation steps (e.g. sampling
heap profiler.)

R=hpayer@chromium.org
BUG=

Committed: https://crrev.com/7b704c4f9bb06386d1fb23d9593296bca9659415
Cr-Commit-Position: refs/heads/master@{#31814}

[ofrobots, 2015-10-12 22:39:42]

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

https://codereview.chromium.org/1404523002/diff/1/src/heap/spaces.cc#newcode1415
src/heap/spaces.cc:1415: } else if (top_on_previous_step_ == 0) {
I did not need to make this change as part of this CL. It is equivalent. It does
pave the way to me remove inline_allocation_limit_step_ completely in a
subsequent CL.

[Hannes Payer (out of office), 2015-10-13 16:51:24]

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

https://codereview.chromium.org/1404523002/diff/1/src/heap/spaces.h#newcode2774
src/heap/spaces.h:2774: intptr_t inline_allocation_limit_step_;
Having a state that belongs to incremental marking or scavenging in spaces is
not nice. I would prefer to keep the abstract value of allocation limit in
spaces and handle actual setting on a higher level.

With this approach, each customer would need its own step counter. That does not
really scale.

[ofrobots, 2015-10-15 19:34:16]

On 2015/10/13 16:51:24, Hannes Payer wrote:
> https://codereview.chromium.org/1404523002/diff/1/src/heap/spaces.h
> File src/heap/spaces.h (right):
> 
>
https://codereview.chromium.org/1404523002/diff/1/src/heap/spaces.h#newcode2774
> src/heap/spaces.h:2774: intptr_t inline_allocation_limit_step_;
> Having a state that belongs to incremental marking or scavenging in spaces is
> not nice. I would prefer to keep the abstract value of allocation limit in
> spaces and handle actual setting on a higher level.
> 
> With this approach, each customer would need its own step counter. That does
not
> really scale.

I have abstracted the observer interface to address this. PTAL.

[Hannes Payer (out of office), 2015-10-16 15:31:16]

hpayer@chromium.org changed reviewers:
+ ulan@chromium.org

[Hannes Payer (out of office), 2015-10-16 15:31:17]

Overall looking good. Adding ulan for a second opinion.

https://codereview.chromium.org/1404523002/diff/20001/src/heap/spaces.h
File src/heap/spaces.h (right):

https://codereview.chromium.org/1404523002/diff/20001/src/heap/spaces.h#newco...
src/heap/spaces.h:2464: class InlineAllocationObserver {
This class needs a description and a few comments. And since it is
unit-testable, can we add a few unit tests.

https://codereview.chromium.org/1404523002/diff/20001/test/cctest/cctest.h
File test/cctest/cctest.h (right):

https://codereview.chromium.org/1404523002/diff/20001/test/cctest/cctest.h#ne...
test/cctest/cctest.h:529: space->DisableInlineAllocationSteps();
Instead of call DisableInlineAllocationSteps, the tests can call
space->DisableInlineAllocationSteps(); directly instead.

[ofrobots, 2015-10-16 21:26:35]

https://codereview.chromium.org/1404523002/diff/20001/src/heap/spaces.h
File src/heap/spaces.h (right):

https://codereview.chromium.org/1404523002/diff/20001/src/heap/spaces.h#newco...
src/heap/spaces.h:2464: class InlineAllocationObserver {
On 2015/10/16 15:31:17, Hannes Payer wrote:
> This class needs a description and a few comments. And since it is
> unit-testable, can we add a few unit tests.

Done.

https://codereview.chromium.org/1404523002/diff/20001/test/cctest/cctest.h
File test/cctest/cctest.h (right):

https://codereview.chromium.org/1404523002/diff/20001/test/cctest/cctest.h#ne...
test/cctest/cctest.h:529: space->DisableInlineAllocationSteps();
On 2015/10/16 15:31:17, Hannes Payer wrote:
> Instead of call DisableInlineAllocationSteps, the tests can call
> space->DisableInlineAllocationSteps(); directly instead.

Done.

[ulan, 2015-10-21 09:06:12]

Looking good.

Any reason for using raw callbacks instead of an Observre interface that is
implemented via virtual functions?

That way we could avoid upcasting the callback parameter since it would be just
be stored in the object that implements the interface.

https://codereview.chromium.org/1404523002/diff/60001/src/heap/spaces.h
File src/heap/spaces.h (right):

https://codereview.chromium.org/1404523002/diff/60001/src/heap/spaces.h#newco...
src/heap/spaces.h:2697: // Allows observation of inline allocation. The callback
gets after every
The callback gets called

[ofrobots, 2015-10-21 13:18:41]

On 2015/10/21 09:06:12, ulan wrote:
> Looking good.
> 
> Any reason for using raw callbacks instead of an Observre interface that is
> implemented via virtual functions?
> 
> That way we could avoid upcasting the callback parameter since it would be
just
> be stored in the object that implements the interface.
> 

I initially implemented it that way, but disliked 
1) A lot of class definition boiler-plate
2) having to allocate and maintaining the lifetime of the object by the callers
of Add/RemoveInlineAllocationObserver.

To keep things nice and simple I switched to callbacks. Let me think a little
bit more about if I want to go back to virtual functions.

[ofrobots, 2015-10-22 02:42:13]

On 2015/10/21 13:18:41, ofrobots wrote:
> On 2015/10/21 09:06:12, ulan wrote:
> > Looking good.
> > 
> > Any reason for using raw callbacks instead of an Observre interface that is
> > implemented via virtual functions?
> > 
> > That way we could avoid upcasting the callback parameter since it would be
> just
> > be stored in the object that implements the interface.
> > 
> 
> I initially implemented it that way, but disliked 
> 1) A lot of class definition boiler-plate
> 2) having to allocate and maintaining the lifetime of the object by the
callers
> of Add/RemoveInlineAllocationObserver.
> 
> To keep things nice and simple I switched to callbacks. Let me think a little
> bit more about if I want to go back to virtual functions.

Switched the observer interface to use virtual functions instead of callbacks. 

PTAL.

[ofrobots, 2015-10-22 02:42:22]

https://codereview.chromium.org/1404523002/diff/60001/src/heap/spaces.h
File src/heap/spaces.h (right):

https://codereview.chromium.org/1404523002/diff/60001/src/heap/spaces.h#newco...
src/heap/spaces.h:2697: // Allows observation of inline allocation. The callback
gets after every
On 2015/10/21 09:06:12, ulan wrote:
> The callback gets called

Done.

[ulan, 2015-10-22 09:02:22]

LGTM with comments. Please wait for Hannes' review before landing.

https://codereview.chromium.org/1404523002/diff/80001/src/heap/incremental-ma...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/1404523002/diff/80001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:589: observer_ = new
IncrementalMarkingObserver(*this, kAllocatedThreshold);
I think we can bind the lifetime of the observer to the lifetime of the
IncrementalMarking object, i.e. create one observer at the beginning and reuse
it.

https://codereview.chromium.org/1404523002/diff/80001/src/heap/spaces.h
File src/heap/spaces.h (right):

https://codereview.chromium.org/1404523002/diff/80001/src/heap/spaces.h#newco...
src/heap/spaces.h:2467: InlineAllocationObserver(const
InlineAllocationObserver&) = delete;
Please use DISALLOW_COPY_AND_ASSIGN() macro.

[Hannes Payer (out of office), 2015-10-22 09:14:14]

LGTM, minor nits. There is one regression with one of your previous CLs:
https://code.google.com/p/chromium/issues/detail?id=541135 Let's fix this one
first. Could you please have a look.

https://codereview.chromium.org/1404523002/diff/80001/src/heap/incremental-ma...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/1404523002/diff/80001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:846: delete observer_;
Let's allocate/delete the observer at heap setup/shutdown.

https://codereview.chromium.org/1404523002/diff/80001/src/heap/incremental-ma...
File src/heap/incremental-marking.h (right):

https://codereview.chromium.org/1404523002/diff/80001/src/heap/incremental-ma...
src/heap/incremental-marking.h:20: class InlineAllocationObserver;
alphabetic order

[ofrobots, 2015-10-26 21:55:01]

Addressed comments from last round. If this looks good, the next step would be
to rebase the CL.

https://codereview.chromium.org/1404523002/diff/80001/src/heap/incremental-ma...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/1404523002/diff/80001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:589: observer_ = new
IncrementalMarkingObserver(*this, kAllocatedThreshold);
On 2015/10/22 09:02:22, ulan wrote:
> I think we can bind the lifetime of the observer to the lifetime of the
> IncrementalMarking object, i.e. create one observer at the beginning and reuse
> it.

Done.

https://codereview.chromium.org/1404523002/diff/80001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:846: delete observer_;
On 2015/10/22 09:14:14, Hannes Payer wrote:
> Let's allocate/delete the observer at heap setup/shutdown.

Done as per Ulan's suggestion above.

https://codereview.chromium.org/1404523002/diff/80001/src/heap/incremental-ma...
File src/heap/incremental-marking.h (right):

https://codereview.chromium.org/1404523002/diff/80001/src/heap/incremental-ma...
src/heap/incremental-marking.h:20: class InlineAllocationObserver;
On 2015/10/22 09:14:14, Hannes Payer wrote:
> alphabetic order

Done.

https://codereview.chromium.org/1404523002/diff/80001/src/heap/spaces.h
File src/heap/spaces.h (right):

https://codereview.chromium.org/1404523002/diff/80001/src/heap/spaces.h#newco...
src/heap/spaces.h:2467: InlineAllocationObserver(const
InlineAllocationObserver&) = delete;
On 2015/10/22 09:02:22, ulan wrote:
> Please use DISALLOW_COPY_AND_ASSIGN() macro.

Done.

[Hannes Payer (out of office), 2015-10-29 14:27:28]

lgtm

[ofrobots, 2015-10-30 00:12:14]

On 2015/10/29 14:27:28, Hannes Payer wrote:
> lgtm

hpayer@: I have rebased this to pick up the revert from
https://codereview.chromium.org/1405043005. Hopefully I used the right workflow
to rebase (I used `git rebase master`). LMK if there was a better way to do
this.

The additional changes required were:
- In Heap::Scavenge: replace the call to
LowerInlineAllocationLimit(new_space_.inline_allocation_limit_step()) with a
call to UpdateInlineAllocationLimitStep
- Make NewSpace::UpdateInlineAllocationLimitStep public to allow the above call.
- Remove method NewSpace::inline_allocation_limit_step() as it is no longer
needed.

Please take a quick look before I submit.

[Hannes Payer (out of office), 2015-10-30 06:12:37]

On 2015/10/30 00:12:14, ofrobots wrote:
> On 2015/10/29 14:27:28, Hannes Payer wrote:
> > lgtm
> 
> hpayer@: I have rebased this to pick up the revert from
> https://codereview.chromium.org/1405043005. Hopefully I used the right
workflow
> to rebase (I used `git rebase master`). LMK if there was a better way to do
> this.
> 
> The additional changes required were:
> - In Heap::Scavenge: replace the call to
> LowerInlineAllocationLimit(new_space_.inline_allocation_limit_step()) with a
> call to UpdateInlineAllocationLimitStep
> - Make NewSpace::UpdateInlineAllocationLimitStep public to allow the above
call.
> - Remove method NewSpace::inline_allocation_limit_step() as it is no longer
> needed.
> 
> Please take a quick look before I submit.

Rebasing breaks the delta but there is not much you can do about that. Change
still looks good. Just go ahead and commit it.

[ofrobots, 2015-10-30 06:15:44]

The CQ bit was checked by ofrobots@google.com

[ofrobots, 2015-10-30 06:15:44]

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/1404523002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-30 06:15:55]

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

[commit-bot: I haz the power, 2015-10-30 06:18:14]

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

[commit-bot: I haz the power, 2015-10-30 06:18:15]

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

[ofrobots, 2015-10-30 13:40:10]

The CQ bit was checked by ofrobots@google.com

[ofrobots, 2015-10-30 13:40:10]

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/1404523002/#ps160001
(title: "prefer static cast over c-style cast")

[commit-bot: I haz the power, 2015-10-30 13:40:21]

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

[commit-bot: I haz the power, 2015-10-30 14:20:36]

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

[commit-bot: I haz the power, 2015-10-30 14:20:37]

Try jobs failed on following builders:
  v8_linux_dbg on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/10259)

[ofrobots, 2015-10-30 18:07:28]

On 2015/10/30 14:20:37, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_linux_dbg on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/10259)

ulan@, hpayer@: this fails on Debug build because set_limit expects the limit to
be aligned. Is there a reason for this requirement? I agree that we should
allocate on aligned addresses, but don't understand the reason why the limit has
to be aligned.

  INLINE(void set_limit(Address limit)) {
    SLOW_DCHECK(limit == NULL ||
                (reinterpret_cast<intptr_t>(limit) & kHeapObjectTagMask) == 0);
    limit_ = limit;
  }

The reason this is failing with my change is that I fixed a fence-post error in
how inline-allocation-limit-step was working. If you set a step_size of 128 then
allocate 128 bytes, you expect to be notified. This wasn't happening before my
change.

I can certainly fix this in the observer logic. However, if you agree that the
assertion in set_limit is overly restrictive, we can avoid some unnecessary
complexity.

[ofrobots, 2015-10-30 23:25:34]

On 2015/10/30 18:07:28, ofrobots wrote:
> On 2015/10/30 14:20:37, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   v8_linux_dbg on tryserver.v8 (JOB_FAILED,
> > http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/10259)
> 
> ulan@, hpayer@: this fails on Debug build because set_limit expects the limit
to
> be aligned. Is there a reason for this requirement? I agree that we should
> allocate on aligned addresses, but don't understand the reason why the limit
has
> to be aligned.
> 
>   INLINE(void set_limit(Address limit)) {
>     SLOW_DCHECK(limit == NULL ||
>                 (reinterpret_cast<intptr_t>(limit) & kHeapObjectTagMask) ==
0);
>     limit_ = limit;
>   }
> 
> The reason this is failing with my change is that I fixed a fence-post error
in
> how inline-allocation-limit-step was working. If you set a step_size of 128
then
> allocate 128 bytes, you expect to be notified. This wasn't happening before my
> change.
> 
> I can certainly fix this in the observer logic. However, if you agree that the
> assertion in set_limit is overly restrictive, we can avoid some unnecessary
> complexity.

I've added patch-set 10 which works around this issue without added complexity,
but with the downside that it makes the definition of the step_size a bit
uglier. I'm fine with this ugliness for now. I don't want to touch set_limit as
part of this CL at all (whatever your disposition on the previous comment is). 

The definition of the step_size is now that the observer notified when `>
step_size` bytes have been allocated as opposed to `>= step_size`.

[ofrobots, 2015-11-02 14:30:30]

The CQ bit was checked by ofrobots@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-02 14:30:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1404523002/160002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1404523002/160002

[commit-bot: I haz the power, 2015-11-02 14:58:12]

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

[commit-bot: I haz the power, 2015-11-02 14:58:13]

Dry run: This issue passed the CQ dry run.

[ofrobots, 2015-11-03 15:07:54]

If there are no objections, I can land this soon.

[ulan, 2015-11-03 15:15:03]

On 2015/11/03 15:07:54, ofrobots wrote:
> If there are no objections, I can land this soon.

The changes in recent patches lgtm.

[ulan, 2015-11-03 15:56:47]

>This fails on Debug build because set_limit expects the limit to
>be aligned. Is there a reason for this requirement?
I don't know the reason. I think it would work without alignment too.

[Hannes Payer (out of office), 2015-11-04 00:25:38]

On 2015/11/03 15:56:47, ulan wrote:
> >This fails on Debug build because set_limit expects the limit to
> >be aligned. Is there a reason for this requirement?
> I don't know the reason. I think it would work without alignment too.

A non-aligned address does not make much sense. Let's make sure it is aligned.

[ofrobots, 2015-11-04 01:45:32]

On 2015/11/04 00:25:38, Hannes Payer wrote:
> On 2015/11/03 15:56:47, ulan wrote:
> > >This fails on Debug build because set_limit expects the limit to
> > >be aligned. Is there a reason for this requirement?
> > I don't know the reason. I think it would work without alignment too.
> 
> A non-aligned address does not make much sense. Let's make sure it is aligned.

Patch-set 10 keeps set_limit aligned, so it is no longer a point of contention
as far as landing this CL is concerned. I will land this tomorrow, unless I hear
explicit objection from you.

[ofrobots, 2015-11-05 04:02:50]

The CQ bit was checked by ofrobots@google.com

[ofrobots, 2015-11-05 04:02:50]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org
Link to the patchset: https://codereview.chromium.org/1404523002/#ps160002
(title: "work-around the requirements that set_limit needs to be aligned")

[commit-bot: I haz the power, 2015-11-05 04:02:56]

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

[commit-bot: I haz the power, 2015-11-05 04:45:23]

Committed patchset #10 (id:160002)

[commit-bot: I haz the power, 2015-11-05 04:45:40]

Patchset 10 (id:??) landed as
https://crrev.com/7b704c4f9bb06386d1fb23d9593296bca9659415
Cr-Commit-Position: refs/heads/master@{#31814}