Reuse empty StoreBufferBlocks.

Reuse empty StoreBufferBlocks.

Keep cache of empty blocks shared among all isolates, limited in size.
Change overflow threshold check to include partially filled blocks.
Make Isolate::store_buffer_ a pointer, now that it's less frequently accessed, to keep the offsets of other fields small


BUG=
R=iposva@google.com

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

[koda, 2015-06-10 13:37:10]

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

[Ivan Posva, 2015-06-16 21:48:20]

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:725: StoreBuffer* store_buffer_;
I don't see the reason why this was necessary. store_buffer.h does not need
anything really from within isolate.h, so it is not likely going to be recursive
includes.

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

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:19: StoreBuffer::List StoreBuffer::global_empty_;
This will lead to statically destructed objects which is something that can
cause issues.

Should be a pointer and allocated in InitOnce().

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:68: MutexLocker ml(mutex_);
ditto from below: Reduce mutex area.

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:87: return partial_.IsEmpty() ? PopEmptyBlock() :
partial_.Pop();
The mutex does not need to be held around the popping of an empty block.

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:93: return global_empty_.IsEmpty() ? new
StoreBufferBlock() : global_empty_.Pop();
The mutex does not need to be held around the malloc and initialization of the
new StoreBufferBlock.

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

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/store_buffer...
runtime/vm/store_buffer.h:42: return pointers_[--top_];
This means that you'll walk through entries in reverse order which can be less
efficient.

If we want to stay with Pop() semantics here, then we should rename Add to Push
and we can make Count private.

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/store_buffer...
runtime/vm/store_buffer.h:121: static void CheckThresholdGlobalEmpty();
How about TrimGlobalEmpty?

[koda, 2015-06-18 17:40:23]

PTAL

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:725: StoreBuffer* store_buffer_;
On 2015/06/16 21:48:19, Ivan Posva wrote:
> I don't see the reason why this was necessary. store_buffer.h does not need
> anything really from within isolate.h, so it is not likely going to be
recursive
> includes.

Not necessary, but the reasoning is in the CL description:

"Make Isolate::store_buffer_ a pointer, now that it's less frequently accessed,
to keep the offsets of other fields small"

Other fields of Isolate will likely still be accessed from generated code, even
after the thread refactoring, and keeping the offsets small enough to use the
best encoding seems like a good general rule.

Also, minimizing dependencies helps keep the VM build time reasonable.

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

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:19: StoreBuffer::List StoreBuffer::global_empty_;
On 2015/06/16 21:48:19, Ivan Posva wrote:
> This will lead to statically destructed objects which is something that can
> cause issues.
> 
> Should be a pointer and allocated in InitOnce().

Done.

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:68: MutexLocker ml(mutex_);
On 2015/06/16 21:48:19, Ivan Posva wrote:
> ditto from below: Reduce mutex area.

Done.

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:87: return partial_.IsEmpty() ? PopEmptyBlock() :
partial_.Pop();
On 2015/06/16 21:48:19, Ivan Posva wrote:
> The mutex does not need to be held around the popping of an empty block.

Done.

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/store_buffer...
runtime/vm/store_buffer.cc:93: return global_empty_.IsEmpty() ? new
StoreBufferBlock() : global_empty_.Pop();
On 2015/06/16 21:48:19, Ivan Posva wrote:
> The mutex does not need to be held around the malloc and initialization of the
> new StoreBufferBlock.

Done.

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

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/store_buffer...
runtime/vm/store_buffer.h:42: return pointers_[--top_];
On 2015/06/16 21:48:19, Ivan Posva wrote:
> This means that you'll walk through entries in reverse order which can be less
> efficient.
> 
> If we want to stay with Pop() semantics here, then we should rename Add to
Push
> and we can make Count private.

Renamed to Push.

Scavenger's stats still need Count to sum up the total (although we could make
StoreBuffer::Blocks also compute+return the count if you prefer).

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/store_buffer...
runtime/vm/store_buffer.h:121: static void CheckThresholdGlobalEmpty();
On 2015/06/16 21:48:19, Ivan Posva wrote:
> How about TrimGlobalEmpty?

Done.

[Ivan Posva, 2015-06-18 18:28:31]

LGTM -ip

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1173043002/diff/80001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:725: StoreBuffer* store_buffer_;
On 2015/06/18 17:40:22, koda wrote:
> On 2015/06/16 21:48:19, Ivan Posva wrote:
> > I don't see the reason why this was necessary. store_buffer.h does not need
> > anything really from within isolate.h, so it is not likely going to be
> recursive
> > includes.
> 
> Not necessary, but the reasoning is in the CL description:
> 
> "Make Isolate::store_buffer_ a pointer, now that it's less frequently
accessed,
> to keep the offsets of other fields small"
> 
> Other fields of Isolate will likely still be accessed from generated code,
even
> after the thread refactoring, and keeping the offsets small enough to use the
> best encoding seems like a good general rule.
> 
> Also, minimizing dependencies helps keep the VM build time reasonable.

My concern was about dependent loads in generated code, which is not going to be
an issue in this case.

[koda, 2015-06-18 23:16:57]

Committed patchset #6 (id:100001) manually as
8b4ac450381bfb02727b35a368b0e4c032bd0c9c (presubmit successful).