Thread-local store buffers, v2

Thread-local store buffers, v2

BUG=
R=iposva@google.com

Committed: https://github.com/dart-lang/sdk/commit/a410c0f993d5f4b2bebaa617c6bbbb137697ec98

[koda, 2015-06-07 04:01:08]

koda@google.com changed reviewers:
+ iposva@google.com

[Ivan Posva, 2015-06-08 13:05:22]

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/store_buffer.cc
File runtime/vm/store_buffer.cc (right):

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:32: while (full_.length() > 0) delete full_.Pop();
while (full_.head != NULL) {
  delete full_.Pop();
}

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:39: while (partial_.length() > 0) {
ditto.

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:58: return (partial_.length() > 0) ? partial_.Pop() :
PopEmpty();
ditto.

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:69: while (length() > 0) {
ditto.

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:103: if (full_.length() > 100) {
Wondering whether we should be using the total number of StoreBufferBlocks in
flight here.

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/store_buffer.h
File runtime/vm/store_buffer.h (right):

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/store_buffer...
runtime/vm/store_buffer.h:81: StoreBufferBlock* PopPartialOrEmpty();
How about just Pop() to get a partial or empty block?

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/store_buffer...
runtime/vm/store_buffer.h:92: class List {
Maybe too much overhead as we do not really need the length?

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/thread.cc
File runtime/vm/thread.cc (right):

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:76: thread->store_buffer_block_ =
isolate->store_buffer()->PopPartialOrEmpty();
Looking at this use here I am revising my suggestion: PopBlock(),
PopEmptyBlock() and PushBlock()

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:126: sb->Push(thread->store_buffer_block_, false);
Please explain in a comment why you are passing false here. The way I understand
it at the moment is because we don't want to schedule a GC during GC
preparation. This point might be lost in the future.

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:133: sb->Push(store_buffer_block_, check_threshold);
The fact that we still have a reference to the StoreBufferBlock in the thread
while calling some function makes me a bit nervous here.

ditto above.

[Ivan Posva, 2015-06-08 13:18:42]

After discussion it looks like we are keeping the List.

Some naming should be adjusted otherwise it LGTM.

Thanks,
-Ivan

[koda, 2015-06-09 13:10:30]

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/store_buffer.cc
File runtime/vm/store_buffer.cc (right):

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:32: while (full_.length() > 0) delete full_.Pop();
On 2015/06/08 13:05:21, Ivan Posva wrote:
> while (full_.head != NULL) {
>   delete full_.Pop();
> }

Done, using IsEmpty.

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:39: while (partial_.length() > 0) {
On 2015/06/08 13:05:21, Ivan Posva wrote:
> ditto.

Done.

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:58: return (partial_.length() > 0) ? partial_.Pop() :
PopEmpty();
On 2015/06/08 13:05:21, Ivan Posva wrote:
> ditto.

Done.

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:69: while (length() > 0) {
On 2015/06/08 13:05:21, Ivan Posva wrote:
> ditto.

Done.

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:103: if (full_.length() > 100) {
On 2015/06/08 13:05:21, Ivan Posva wrote:
> Wondering whether we should be using the total number of StoreBufferBlocks in
> flight here.

Yes, I suppose there could be a temporary explosion of short-lived threads,
which would then leave a bunch of almost empty blocks sitting unused. Will roll
that into the next CL with shared list of empty blocks.

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/store_buffer.h
File runtime/vm/store_buffer.h (right):

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/store_buffer...
runtime/vm/store_buffer.h:92: class List {
On 2015/06/08 13:05:21, Ivan Posva wrote:
> Maybe too much overhead as we do not really need the length?

This abstraction will be useful also for the shared list of empty blocks. Added
IsEmpty() method and used it instead of length() in most places.

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/thread.cc
File runtime/vm/thread.cc (right):

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:76: thread->store_buffer_block_ =
isolate->store_buffer()->PopPartialOrEmpty();
On 2015/06/08 13:05:21, Ivan Posva wrote:
> Looking at this use here I am revising my suggestion: PopBlock(),
> PopEmptyBlock() and PushBlock()

Done.

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:126: sb->Push(thread->store_buffer_block_, false);
On 2015/06/08 13:05:21, Ivan Posva wrote:
> Please explain in a comment why you are passing false here. The way I
understand
> it at the moment is because we don't want to schedule a GC during GC
> preparation. This point might be lost in the future.

Done.

https://codereview.chromium.org/1168483003/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:133: sb->Push(store_buffer_block_, check_threshold);
On 2015/06/08 13:05:21, Ivan Posva wrote:
> The fact that we still have a reference to the StoreBufferBlock in the thread
> while calling some function makes me a bit nervous here.
> 
> ditto above.

Done.

[koda, 2015-06-09 16:33:50]

Committed patchset #4 (id:60001) manually as
a410c0f993d5f4b2bebaa617c6bbbb137697ec98 (presubmit successful).