[heap] Concurrent store buffer processing.

[heap] Concurrent store buffer processing.

BUG=chromium:648973, chromium:648568
Committed: https://crrev.com/50a5853f0de1b23f2b41f07d4856397c838ecbd0
Cr-Commit-Position: refs/heads/master@{#40642}

[Hannes Payer (out of office), 2016-10-27 11:01:55]

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

[commit-bot: I haz the power, 2016-10-27 11:02:01]

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-10-27 11:37:56]

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

[commit-bot: I haz the power, 2016-10-27 11:37:57]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11593)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[Hannes Payer (out of office), 2016-10-27 13:17:04]

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

[commit-bot: I haz the power, 2016-10-27 13:17:07]

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-10-27 13:23:37]

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

[commit-bot: I haz the power, 2016-10-27 13:23:38]

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

[Hannes Payer (out of office), 2016-10-27 13:25:09]

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

[commit-bot: I haz the power, 2016-10-27 13:25:17]

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

[Hannes Payer (out of office), 2016-10-27 13:43:12]

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

[commit-bot: I haz the power, 2016-10-27 13:43:19]

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

[Hannes Payer (out of office), 2016-10-27 13:49:13]

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

[Hannes Payer (out of office), 2016-10-27 13:49:14]

I was aiming for a wait-free option for the main thread, but that seems to be
inherently not possible. The current solution could still be made more fine
grained from a synchronization perspective, i.e. the main thread would never
need to grab the lock when transferring entries to the remembered set which
would add a little complexity to the slot set allocation. However, I would
prefer simplicity here over a finer grained synchronization mechanism. Please
have a look, but let the bots go green first :)

[commit-bot: I haz the power, 2016-10-27 13:51:48]

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

[commit-bot: I haz the power, 2016-10-27 13:51:49]

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

[Hannes Payer (out of office), 2016-10-27 13:56:40]

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

[commit-bot: I haz the power, 2016-10-27 13:56:49]

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-10-27 13:59:59]

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

[commit-bot: I haz the power, 2016-10-27 13:59:59]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[Hannes Payer (out of office), 2016-10-27 19:53:08]

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

[commit-bot: I haz the power, 2016-10-27 19:53:14]

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

[Hannes Payer (out of office), 2016-10-27 19:59:51]

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

[commit-bot: I haz the power, 2016-10-27 19:59:54]

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-10-27 20:48:02]

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

[commit-bot: I haz the power, 2016-10-27 20:48:03]

Dry run: This issue passed the CQ dry run.

[Hannes Payer (out of office), 2016-10-27 21:15:52]

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

[commit-bot: I haz the power, 2016-10-27 21:15:58]

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

[Hannes Payer (out of office), 2016-10-27 21:16:31]

PTAL

[Hannes Payer (out of office), 2016-10-27 21:39:28]

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

[commit-bot: I haz the power, 2016-10-27 21:39:39]

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

[Hannes Payer (out of office), 2016-10-27 21:52:32]

Patchset #10 (id:180001) has been deleted

[commit-bot: I haz the power, 2016-10-27 21:53:50]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[commit-bot: I haz the power, 2016-10-27 21:55:40]

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

[commit-bot: I haz the power, 2016-10-27 21:55:40]

Dry run: This issue passed the CQ dry run.

[Hannes Payer (out of office), 2016-10-28 06:43:02]

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

[commit-bot: I haz the power, 2016-10-28 06:43:13]

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

[Hannes Payer (out of office), 2016-10-28 06:50:27]

Description was changed from

==========
[heap] Concurrent store buffer processing.

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

to

==========
[heap] Concurrent store buffer processing.

BUG=chromium:648973, chromium:648568
==========

[commit-bot: I haz the power, 2016-10-28 07:09:59]

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

[commit-bot: I haz the power, 2016-10-28 07:10:00]

Dry run: Try jobs failed on following builders:
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/16999)
  v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)

[Hannes Payer (out of office), 2016-10-28 07:15:04]

https://codereview.chromium.org/2453673003/diff/200001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/2453673003/diff/200001/src/heap/mark-compact....
src/heap/mark-compact.cc:3443:
heap()->store_buffer()->MoveAllEntriesToRememberedSet();
Instead of doing this, we could also filter out directly in the remembered set
when slots are invalidated in the GC.

[Hannes Payer (out of office), 2016-10-28 07:41:48]

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

[commit-bot: I haz the power, 2016-10-28 07:41:53]

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-10-28 08:09:41]

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

[commit-bot: I haz the power, 2016-10-28 08:09:42]

Dry run: This issue passed the CQ dry run.

[ulan, 2016-10-28 09:18:02]

Looking good. First round of comments (I still need to check concurrent
insertion to and removal from the remembered set).

https://codereview.chromium.org/2453673003/diff/220001/src/heap/store-buffer.cc
File src/heap/store-buffer.cc (right):

https://codereview.chromium.org/2453673003/diff/220001/src/heap/store-buffer....
src/heap/store-buffer.cc:91: if (!task_running_.Value()) {
It could be that the task is almost finishing, but task_running_ is true. In
that case we will not start a new task and the store buffer will have to be
processed by the main thread.

Is it expected?

Since we already use mutex, I wonder if it is worth moving the mutex one level
up to FlipStoreBuffers, ConcurrentlyProcessStoreBuffer, and
MoveAllEntriesToRememberedSet? I think the lock contention would be the same
because the main work is done in MoveEntriesToRememberedSet.

The task_running would be normal variable (not atomic) and we would be able to
use current_ from the task and drain the other buffer using top_[(current_ + 1)
% kStoreBuffers]. There would be no need for atomic lazy_top_. wdyt?

https://codereview.chromium.org/2453673003/diff/220001/src/heap/store-buffer.h
File src/heap/store-buffer.h (right):

https://codereview.chromium.org/2453673003/diff/220001/src/heap/store-buffer....
src/heap/store-buffer.h:19: // code. Moreover, it stores invalid old-to-new
slots with two two entries.
Nit: with _two_ two entries.

https://codereview.chromium.org/2453673003/diff/220001/src/heap/store-buffer....
src/heap/store-buffer.h:111: // added from the generated code. We have to chunks
of store buffers.
typo: We have _two_

https://codereview.chromium.org/2453673003/diff/220001/src/heap/store-buffer....
src/heap/store-buffer.h:116: base::AtomicValue<Address*>
lazy_top_[kStoreBuffers];
Is it an invariant that at most one lazy_top_ is non-null at any moment?
If so could you please add a comment. I think it is needed for
ConcurrentlyProcessStoreBuffer.

[Hannes Payer (out of office), 2016-10-28 09:58:24]

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

[Hannes Payer (out of office), 2016-10-28 09:58:33]

https://codereview.chromium.org/2453673003/diff/220001/src/heap/store-buffer.cc
File src/heap/store-buffer.cc (right):

https://codereview.chromium.org/2453673003/diff/220001/src/heap/store-buffer....
src/heap/store-buffer.cc:91: if (!task_running_.Value()) {
On 2016/10/28 09:18:01, ulan wrote:
> It could be that the task is almost finishing, but task_running_ is true. In
> that case we will not start a new task and the store buffer will have to be
> processed by the main thread.
> 
> Is it expected?
> 
> Since we already use mutex, I wonder if it is worth moving the mutex one level
> up to FlipStoreBuffers, ConcurrentlyProcessStoreBuffer, and
> MoveAllEntriesToRememberedSet? I think the lock contention would be the same
> because the main work is done in MoveEntriesToRememberedSet.
> 
> The task_running would be normal variable (not atomic) and we would be able to
> use current_ from the task and drain the other buffer using top_[(current_ +
1)
> % kStoreBuffers]. There would be no need for atomic lazy_top_. wdyt?
> 

I optimized for the case that Flip would not need to take a lock when the other
buffer was already processed. However, that may be a micro optimization not
necessary. The solution you outlined is even coarser grained from a locking
perspective, which results in simpler code. Since it is not clear what the
optimization buys us I will go with the simpler solution.

https://codereview.chromium.org/2453673003/diff/220001/src/heap/store-buffer.h
File src/heap/store-buffer.h (right):

https://codereview.chromium.org/2453673003/diff/220001/src/heap/store-buffer....
src/heap/store-buffer.h:19: // code. Moreover, it stores invalid old-to-new
slots with two two entries.
On 2016/10/28 09:18:02, ulan wrote:
> Nit: with _two_ two entries.

Done.

https://codereview.chromium.org/2453673003/diff/220001/src/heap/store-buffer....
src/heap/store-buffer.h:111: // added from the generated code. We have to chunks
of store buffers.
On 2016/10/28 09:18:02, ulan wrote:
> typo: We have _two_

Done.

https://codereview.chromium.org/2453673003/diff/220001/src/heap/store-buffer....
src/heap/store-buffer.h:116: base::AtomicValue<Address*>
lazy_top_[kStoreBuffers];
On 2016/10/28 09:18:02, ulan wrote:
> Is it an invariant that at most one lazy_top_ is non-null at any moment?
> If so could you please add a comment. I think it is needed for
> ConcurrentlyProcessStoreBuffer.

Yes, that is correct. Done.

[commit-bot: I haz the power, 2016-10-28 09:58:35]

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

[ulan, 2016-10-28 10:12:29]

lgtm!

https://codereview.chromium.org/2453673003/diff/240001/src/heap/store-buffer.cc
File src/heap/store-buffer.cc (right):

https://codereview.chromium.org/2453673003/diff/240001/src/heap/store-buffer....
src/heap/store-buffer.cc:84: MoveEntriesToRememberedSet(other);
Since MoveEntriesToRememberedSet already checks for lazy_top[i] == nullptr, we
can simplify this:
MoveEntriesToRememberedSet((current_ + 1) % kStoreBuffers);
The same for two places below.

[Hannes Payer (out of office), 2016-10-28 10:15:25]

https://codereview.chromium.org/2453673003/diff/240001/src/heap/store-buffer.cc
File src/heap/store-buffer.cc (right):

https://codereview.chromium.org/2453673003/diff/240001/src/heap/store-buffer....
src/heap/store-buffer.cc:84: MoveEntriesToRememberedSet(other);
On 2016/10/28 10:12:29, ulan wrote:
> Since MoveEntriesToRememberedSet already checks for lazy_top[i] == nullptr, we
> can simplify this:
> MoveEntriesToRememberedSet((current_ + 1) % kStoreBuffers);
> The same for two places below.

Done.

[Hannes Payer (out of office), 2016-10-28 10:15:29]

The CQ bit was checked by hpayer@chromium.org

[Hannes Payer (out of office), 2016-10-28 10:15:30]

The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org
Link to the patchset: https://codereview.chromium.org/2453673003/#ps260001
(title: "remove check")

[commit-bot: I haz the power, 2016-10-28 10:15:34]

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-10-28 10:40:10]

Description was changed from

==========
[heap] Concurrent store buffer processing.

BUG=chromium:648973, chromium:648568
==========

to

==========
[heap] Concurrent store buffer processing.

BUG=chromium:648973, chromium:648568
==========

[commit-bot: I haz the power, 2016-10-28 10:40:12]

Committed patchset #13 (id:260001)

[Michael Achenbach, 2016-10-28 16:45:19]

A revert of this CL (patchset #13 id:260001) has been created in
https://codereview.chromium.org/2449853010/ by machenbach@chromium.org.

The reason for reverting is: Seems to block rolling:
https://codereview.chromium.org/2447393005/.

[commit-bot: I haz the power, 2016-11-17 22:16:37]

Description was changed from

==========
[heap] Concurrent store buffer processing.

BUG=chromium:648973, chromium:648568
==========

to

==========
[heap] Concurrent store buffer processing.

BUG=chromium:648973, chromium:648568

Committed: https://crrev.com/50a5853f0de1b23f2b41f07d4856397c838ecbd0
Cr-Commit-Position: refs/heads/master@{#40642}
==========

[commit-bot: I haz the power, 2016-11-17 22:16:39]

Patchset 13 (id:??) landed as
https://crrev.com/50a5853f0de1b23f2b41f07d4856397c838ecbd0
Cr-Commit-Position: refs/heads/master@{#40642}