[heap] New heuristics for incremental marking step size.

[heap] New heuristics for incremental marking step size.

This patch simplifies code for speeding up marking and
removes write barrier counter.

The step size is now computed based in two parts:
- bytes to mark in order to keep up with allocation,
- bytes to mark in order to make progress.

BUG=chromium:616434, chromium:646139, chromium:644819
LOG=NO

Committed: https://crrev.com/1beb89f24cb01663843a7519921e7caf0910c760
Cr-Commit-Position: refs/heads/master@{#39827}

[ulan, 2016-09-22 14:04:09]

Patchset #1 (id:1) has been deleted

[ulan, 2016-09-22 14:04:17]

Patchset #1 (id:20001) has been deleted

[ulan, 2016-09-22 14:04:24]

Patchset #1 (id:40001) has been deleted

[ulan, 2016-09-22 14:04:35]

Patchset #1 (id:60001) has been deleted

[ulan, 2016-09-22 14:04:46]

Patchset #1 (id:80001) has been deleted

[ulan, 2016-09-22 14:21:40]

Description was changed from

==========
[heap] Incremental marking speed adjustment
BUG=
==========

to

==========
[heap] New heuristics for incremental marking step size.

This patch simplifies code for speeding up marking and
removes write barrier counter.

The step size is now computed based in two parts:
- bytes to mark in order to keep up with allocation,
- bytes to mark in order to make progress.

BUG=chromium:616434, chromium:646139, chromium:644819
LOG=NO
==========

[ulan, 2016-09-22 14:23:25]

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

[ulan, 2016-09-22 14:26:20]

ptal

[Hannes Payer (out of office), 2016-09-26 14:32:06]

Pretty! Looking good. Just minor things.

https://codereview.chromium.org/2359903002/diff/250001/src/heap/incremental-m...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/2359903002/diff/250001/src/heap/incremental-m...
src/heap/incremental-marking.cc:68: IncrementalMarking* marking =
isolate->heap()->incremental_marking();
Nice. Change to: isolate->heap()->incremental_marking()->RecordWrite(obj, slot,
*slot);

https://codereview.chromium.org/2359903002/diff/250001/src/heap/incremental-m...
src/heap/incremental-marking.cc:514:
heap_->new_space()->AddAllocationObserver(&new_generation_observer_);
Use the space iterator since we are iterating over all spaces.

https://codereview.chromium.org/2359903002/diff/250001/src/heap/incremental-m...
src/heap/incremental-marking.cc:985:
heap_->old_space()->RemoveAllocationObserver(&old_generation_observer_);
Use the space iterator since we are iterating over all spaces.

https://codereview.chromium.org/2359903002/diff/250001/src/heap/incremental-m...
src/heap/incremental-marking.cc:1082: size_t current_counter =
heap_->OldGenerationAllocationCounter();
Can we not use the same name for OldGenerationAllocationCounter and
old_generation_allocation_counter_ for better readability?

https://codereview.chromium.org/2359903002/diff/250001/src/heap/incremental-m...
src/heap/incremental-marking.cc:1117: bytes_to_process = Min(bytes_to_process,
max_step_size);
Can we think of a way to avoid this big step after Scavenge? Or do you think
this is not an issue?

[Hannes Payer (out of office), 2016-09-27 07:44:59]

On 2016/09/26 14:32:06, Hannes Payer wrote:
> Pretty! Looking good. Just minor things.
> 
>
https://codereview.chromium.org/2359903002/diff/250001/src/heap/incremental-m...
> File src/heap/incremental-marking.cc (right):
> 
>
https://codereview.chromium.org/2359903002/diff/250001/src/heap/incremental-m...
> src/heap/incremental-marking.cc:68: IncrementalMarking* marking =
> isolate->heap()->incremental_marking();
> Nice. Change to: isolate->heap()->incremental_marking()->RecordWrite(obj,
slot,
> *slot);
> 
>
https://codereview.chromium.org/2359903002/diff/250001/src/heap/incremental-m...
> src/heap/incremental-marking.cc:514:
> heap_->new_space()->AddAllocationObserver(&new_generation_observer_);
> Use the space iterator since we are iterating over all spaces.
> 
>
https://codereview.chromium.org/2359903002/diff/250001/src/heap/incremental-m...
> src/heap/incremental-marking.cc:985:
> heap_->old_space()->RemoveAllocationObserver(&old_generation_observer_);
> Use the space iterator since we are iterating over all spaces.
> 
>
https://codereview.chromium.org/2359903002/diff/250001/src/heap/incremental-m...
> src/heap/incremental-marking.cc:1082: size_t current_counter =
> heap_->OldGenerationAllocationCounter();
> Can we not use the same name for OldGenerationAllocationCounter and
> old_generation_allocation_counter_ for better readability?
> 
>
https://codereview.chromium.org/2359903002/diff/250001/src/heap/incremental-m...
> src/heap/incremental-marking.cc:1117: bytes_to_process = Min(bytes_to_process,
> max_step_size);
> Can we think of a way to avoid this big step after Scavenge? Or do you think
> this is not an issue?

As discussed offline, we can address the step after scavenge issue after landing
this CL. So, LGTM.

[ulan, 2016-09-27 17:02:09]

https://codereview.chromium.org/2359903002/diff/250001/src/heap/incremental-m...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/2359903002/diff/250001/src/heap/incremental-m...
src/heap/incremental-marking.cc:68: IncrementalMarking* marking =
isolate->heap()->incremental_marking();
On 2016/09/26 14:32:05, Hannes Payer wrote:
> Nice. Change to: isolate->heap()->incremental_marking()->RecordWrite(obj,
slot,
> *slot);

Done.

https://codereview.chromium.org/2359903002/diff/250001/src/heap/incremental-m...
src/heap/incremental-marking.cc:985:
heap_->old_space()->RemoveAllocationObserver(&old_generation_observer_);
On 2016/09/26 14:32:05, Hannes Payer wrote:
> Use the space iterator since we are iterating over all spaces.

Done.

https://codereview.chromium.org/2359903002/diff/250001/src/heap/incremental-m...
src/heap/incremental-marking.cc:1082: size_t current_counter =
heap_->OldGenerationAllocationCounter();
On 2016/09/26 14:32:05, Hannes Payer wrote:
> Can we not use the same name for OldGenerationAllocationCounter and
> old_generation_allocation_counter_ for better readability?

Done.

https://codereview.chromium.org/2359903002/diff/250001/src/heap/incremental-m...
src/heap/incremental-marking.cc:1117: bytes_to_process = Min(bytes_to_process,
max_step_size);
On 2016/09/26 14:32:05, Hannes Payer wrote:
> Can we think of a way to avoid this big step after Scavenge? Or do you think
> this is not an issue?

Acknowledged.

[ulan, 2016-09-27 17:02:14]

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

[commit-bot: I haz the power, 2016-09-27 17:02:18]

Dry run: 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, 2016-09-27 17:31:29]

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

[commit-bot: I haz the power, 2016-09-27 17:31:30]

Dry run: This issue passed the CQ dry run.

[ulan, 2016-09-28 13:24:44]

The CQ bit was checked by ulan@chromium.org

[ulan, 2016-09-28 13:24:44]

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/2359903002/#ps290001
(title: "use space iterator")

[commit-bot: I haz the power, 2016-09-28 13:24:48]

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, 2016-09-28 13:27:47]

Description was changed from

==========
[heap] New heuristics for incremental marking step size.

This patch simplifies code for speeding up marking and
removes write barrier counter.

The step size is now computed based in two parts:
- bytes to mark in order to keep up with allocation,
- bytes to mark in order to make progress.

BUG=chromium:616434, chromium:646139, chromium:644819
LOG=NO
==========

to

==========
[heap] New heuristics for incremental marking step size.

This patch simplifies code for speeding up marking and
removes write barrier counter.

The step size is now computed based in two parts:
- bytes to mark in order to keep up with allocation,
- bytes to mark in order to make progress.

BUG=chromium:616434, chromium:646139, chromium:644819
LOG=NO
==========

[commit-bot: I haz the power, 2016-09-28 13:27:49]

Committed patchset #11 (id:290001)

[commit-bot: I haz the power, 2016-09-28 13:28:04]

Description was changed from

==========
[heap] New heuristics for incremental marking step size.

This patch simplifies code for speeding up marking and
removes write barrier counter.

The step size is now computed based in two parts:
- bytes to mark in order to keep up with allocation,
- bytes to mark in order to make progress.

BUG=chromium:616434, chromium:646139, chromium:644819
LOG=NO
==========

to

==========
[heap] New heuristics for incremental marking step size.

This patch simplifies code for speeding up marking and
removes write barrier counter.

The step size is now computed based in two parts:
- bytes to mark in order to keep up with allocation,
- bytes to mark in order to make progress.

BUG=chromium:616434, chromium:646139, chromium:644819
LOG=NO

Committed: https://crrev.com/1beb89f24cb01663843a7519921e7caf0910c760
Cr-Commit-Position: refs/heads/master@{#39827}
==========

[commit-bot: I haz the power, 2016-09-28 13:28:05]

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

[Michael Achenbach, 2016-09-29 09:10:24]

FYI: Looks like this broke mips compile:
https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder...

[Michael Achenbach, 2016-09-29 09:10:48]

On 2016/09/29 09:10:24, machenbach (slow) wrote:
> FYI: Looks like this broke mips compile:
>
https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder...

No reverting needed, but fix welcome...

[ulan, 2016-09-29 10:08:30]

On 2016/09/29 09:10:48, machenbach (slow) wrote:
> On 2016/09/29 09:10:24, machenbach (slow) wrote:
> > FYI: Looks like this broke mips compile:
> >
>
https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder...
> 
> No reverting needed, but fix welcome...

fix in CQ