Perform scavenge in idle tasks.

Perform scavenge in idle tasks.

BUG=chromium:490559
LOG=NO

Committed: https://crrev.com/6256dc53cc62cd720c3b1869fcb04f1ab2110d62
Cr-Commit-Position: refs/heads/master@{#30944}

[ulan, 2015-09-17 16:08:35]

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

[ulan, 2015-09-17 16:08:36]

PTAL. Unit-tests for the static functions in ScavengeJob are coming soon.

[Hannes Payer (out of office), 2015-09-21 09:00:57]

I like this change a lot! A few comments... And waiting for tests.

https://codereview.chromium.org/1352453004/diff/100001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1352453004/diff/100001/src/heap/heap.cc#newco...
src/heap/heap.cc:4995: void Heap::ResetInlineAllocationLimit() {
For consistency, you have to call this method also when the heap/space is set
up. Otherwise you will not do idle scavenges before the first gc.

https://codereview.chromium.org/1352453004/diff/100001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1352453004/diff/100001/src/heap/heap.h#newcode18
src/heap/heap.h:18: #include "src/heap/scavenge-job.h"
Move this include to heap.cc

https://codereview.chromium.org/1352453004/diff/100001/src/heap/heap.h#newcod...
src/heap/heap.h:1193: void ResetInlineAllocationLimit();
We should not expose them on the public heap API.

https://codereview.chromium.org/1352453004/diff/100001/src/heap/heap.h#newcod...
src/heap/heap.h:1220: void ScheduleIdleScavenge(int bytes_allocated);
This one should be private as well.

https://codereview.chromium.org/1352453004/diff/100001/src/heap/heap.h#newcod...
src/heap/heap.h:2276: ScavengeJob scavenge_job_;
If you make ScavengeJob a ScavengeJob*, you don't have to include it in heap.h

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

https://codereview.chromium.org/1352453004/diff/100001/src/heap/spaces.cc#new...
src/heap/spaces.cc:1471: } else if (inline_allocation_limit_step() == 0) {
This case is now dead code.

https://codereview.chromium.org/1352453004/diff/100001/src/heap/spaces.cc#new...
src/heap/spaces.cc:1555: // Either the limit has been lowered because linear
allocation was disabled
Update this comment (maybe some other comments also needs updates after your
change).

https://codereview.chromium.org/1352453004/diff/100001/src/heap/spaces.cc#new...
src/heap/spaces.cc:1560: heap()->ScheduleIdleScavenge(bytes_allocated);
Hmm, when incremental marking is on, steps will be really small and we are going
to trigger many idle tasks for scavenge. I don't think we should do that. Can we
just invoke this method on ScavengeJob::kBytesAllocatedBeforeNextIdleTask
allocations? WDYT?

[ulan, 2015-09-23 09:58:39]

Thank you for review. I added tests

https://codereview.chromium.org/1352453004/diff/100001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1352453004/diff/100001/src/heap/heap.cc#newco...
src/heap/heap.cc:4995: void Heap::ResetInlineAllocationLimit() {
On 2015/09/21 09:00:56, Hannes Payer wrote:
> For consistency, you have to call this method also when the heap/space is set
> up. Otherwise you will not do idle scavenges before the first gc.

Yup, I call it in Heap::SetUp below (line 5100)

https://codereview.chromium.org/1352453004/diff/100001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1352453004/diff/100001/src/heap/heap.h#newcod...
src/heap/heap.h:1193: void ResetInlineAllocationLimit();
On 2015/09/21 09:00:57, Hannes Payer wrote:
> We should not expose them on the public heap API.

Incremental marker calls these functions.

https://codereview.chromium.org/1352453004/diff/100001/src/heap/heap.h#newcod...
src/heap/heap.h:1220: void ScheduleIdleScavenge(int bytes_allocated);
On 2015/09/21 09:00:57, Hannes Payer wrote:
> This one should be private as well.

This is called by NewSpace on interrupt.

https://codereview.chromium.org/1352453004/diff/100001/src/heap/heap.h#newcod...
src/heap/heap.h:2276: ScavengeJob scavenge_job_;
On 2015/09/21 09:00:57, Hannes Payer wrote:
> If you make ScavengeJob a ScavengeJob*, you don't have to include it in heap.h

Done.

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

https://codereview.chromium.org/1352453004/diff/100001/src/heap/spaces.cc#new...
src/heap/spaces.cc:1471: } else if (inline_allocation_limit_step() == 0) {
On 2015/09/21 09:00:57, Hannes Payer wrote:
> This case is now dead code.

Done.

https://codereview.chromium.org/1352453004/diff/100001/src/heap/spaces.cc#new...
src/heap/spaces.cc:1555: // Either the limit has been lowered because linear
allocation was disabled
On 2015/09/21 09:00:57, Hannes Payer wrote:
> Update this comment (maybe some other comments also needs updates after your
> change).

Done.

https://codereview.chromium.org/1352453004/diff/100001/src/heap/spaces.cc#new...
src/heap/spaces.cc:1560: heap()->ScheduleIdleScavenge(bytes_allocated);
On 2015/09/21 09:00:57, Hannes Payer wrote:
> Hmm, when incremental marking is on, steps will be really small and we are
going
> to trigger many idle tasks for scavenge. I don't think we should do that. Can
we
> just invoke this method on ScavengeJob::kBytesAllocatedBeforeNextIdleTask
> allocations? WDYT?

The function already checks the bytes allocated. Changed the name accordingly.

[Hannes Payer (out of office), 2015-09-23 14:16:40]

LGTM

https://codereview.chromium.org/1352453004/diff/100001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1352453004/diff/100001/src/heap/heap.cc#newco...
src/heap/heap.cc:4995: void Heap::ResetInlineAllocationLimit() {
On 2015/09/23 09:58:38, ulan wrote:
> On 2015/09/21 09:00:56, Hannes Payer wrote:
> > For consistency, you have to call this method also when the heap/space is
set
> > up. Otherwise you will not do idle scavenges before the first gc.
> 
> Yup, I call it in Heap::SetUp below (line 5100)

Acknowledged.

https://codereview.chromium.org/1352453004/diff/100001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1352453004/diff/100001/src/heap/heap.h#newcod...
src/heap/heap.h:1193: void ResetInlineAllocationLimit();
On 2015/09/23 09:58:38, ulan wrote:
> On 2015/09/21 09:00:57, Hannes Payer wrote:
> > We should not expose them on the public heap API.
> 
> Incremental marker calls these functions.

As discussed offline, we make them friends and do not expose them publicly.

https://codereview.chromium.org/1352453004/diff/180001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1352453004/diff/180001/src/heap/heap.h#newcod...
src/heap/heap.h:430: class ScavengeJob;
class Scavenger;
class ScavengeJob;

https://codereview.chromium.org/1352453004/diff/180001/src/heap/heap.h#newcod...
src/heap/heap.h:1832: void ScheduleIdleScavengeIfNeeded(int bytes_allocated);
Can you move it to the Idle notification section, below (line 1893)?

https://codereview.chromium.org/1352453004/diff/180001/src/heap/heap.h#newcod...
src/heap/heap.h:1888: // Update the inline allocation limit.
Make it a separate // Inline allocation. ===== section

[ulan, 2015-09-23 15:05:16]

https://codereview.chromium.org/1352453004/diff/180001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1352453004/diff/180001/src/heap/heap.h#newcod...
src/heap/heap.h:430: class ScavengeJob;
On 2015/09/23 14:16:39, Hannes Payer wrote:
> class Scavenger;
> class ScavengeJob;

Done.

https://codereview.chromium.org/1352453004/diff/180001/src/heap/heap.h#newcod...
src/heap/heap.h:1832: void ScheduleIdleScavengeIfNeeded(int bytes_allocated);
On 2015/09/23 14:16:39, Hannes Payer wrote:
> Can you move it to the Idle notification section, below (line 1893)?

Done.

https://codereview.chromium.org/1352453004/diff/180001/src/heap/heap.h#newcod...
src/heap/heap.h:1888: // Update the inline allocation limit.
On 2015/09/23 14:16:39, Hannes Payer wrote:
> Make it a separate // Inline allocation. ===== section

Done.

[ulan, 2015-09-24 11:51:22]

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

[commit-bot: I haz the power, 2015-09-24 11:51:35]

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

[commit-bot: I haz the power, 2015-09-24 11:54:33]

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

[commit-bot: I haz the power, 2015-09-24 11:54:34]

Dry run: Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/10023)

[ulan, 2015-09-24 12:04:27]

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

[commit-bot: I haz the power, 2015-09-24 12:04:38]

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

[commit-bot: I haz the power, 2015-09-24 12:14:57]

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

[commit-bot: I haz the power, 2015-09-24 12:14:57]

Dry run: Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/10024)

[ulan, 2015-09-24 13:20:54]

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

[commit-bot: I haz the power, 2015-09-24 13:21:01]

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

[ulan, 2015-09-24 13:36:27]

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

[commit-bot: I haz the power, 2015-09-24 13:36:33]

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

[commit-bot: I haz the power, 2015-09-24 14:09:38]

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

[commit-bot: I haz the power, 2015-09-24 14:09:39]

Dry run: This issue passed the CQ dry run.

[ulan, 2015-09-25 12:33:29]

The CQ bit was checked by ulan@chromium.org

[ulan, 2015-09-25 12:33:29]

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/1352453004/#ps260001
(title: "More test fixes")

[commit-bot: I haz the power, 2015-09-25 12:33:43]

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

[commit-bot: I haz the power, 2015-09-25 12:35:43]

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

[commit-bot: I haz the power, 2015-09-25 12:35:44]

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

[ulan, 2015-09-25 12:44:07]

The CQ bit was checked by ulan@chromium.org

[ulan, 2015-09-25 12:44:07]

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

[commit-bot: I haz the power, 2015-09-25 12:44:17]

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

[ulan, 2015-09-25 13:06:32]

The CQ bit was unchecked by ulan@chromium.org

[ulan, 2015-09-25 14:20:34]

The CQ bit was checked by ulan@chromium.org

[ulan, 2015-09-25 14:20:34]

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

[commit-bot: I haz the power, 2015-09-25 14:20:42]

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

[commit-bot: I haz the power, 2015-09-25 14:49:13]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-09-25 14:49:32]

Patchset 16 (id:??) landed as
https://crrev.com/6256dc53cc62cd720c3b1869fcb04f1ab2110d62
Cr-Commit-Position: refs/heads/master@{#30944}