Synchronize store buffer processing and concurrent sweeping.

Synchronize store buffer processing and concurrent sweeping.

BUG=
R=jarin@chromium.org, mstarzinger@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=20582

[Michael Starzinger, 2014-04-08 10:35:30]

LGTM (with one comment). Although I have to admit it caused a slight
complexity-induced brain explosion. :/

https://codereview.chromium.org/227533006/diff/20001/src/store-buffer.cc
File src/store-buffer.cc (right):

https://codereview.chromium.org/227533006/diff/20001/src/store-buffer.cc#newc...
src/store-buffer.cc:603:
NoBarrier_Load(reinterpret_cast<AtomicWord*>(current)));
I don't understand the reason for the atomic read of "current" here. Is it
really necessary? Otherwise let's drop it.

[Hannes Payer (out of office), 2014-04-08 10:45:13]

https://codereview.chromium.org/227533006/diff/20001/src/store-buffer.cc
File src/store-buffer.cc (right):

https://codereview.chromium.org/227533006/diff/20001/src/store-buffer.cc#newc...
src/store-buffer.cc:603:
NoBarrier_Load(reinterpret_cast<AtomicWord*>(current)));
On 2014/04/08 10:35:31, Michael Starzinger wrote:
> I don't understand the reason for the atomic read of "current" here. Is it
> really necessary? Otherwise let's drop it.
Done.

Nope, not necessary. The sweeper is not touching the store buffer. This change
slipped in somehow. I removed it.

[Jarin, 2014-04-08 13:00:53]

Good progress, but as we discussed, it needs a bit more work. Thanks.

https://codereview.chromium.org/227533006/diff/20001/src/spaces.cc
File src/spaces.cc (right):

https://codereview.chromium.org/227533006/diff/20001/src/spaces.cc#newcode2026
src/spaces.cc:2026:
set_map_no_write_barrier(heap->raw_unchecked_free_space_map());
Here we should do unsynchronized write of size and then release write of the
map, no?

As it is written here, other threads could still see the new map with a bogus
size.

https://codereview.chromium.org/227533006/diff/20001/src/spaces.cc#newcode2070
src/spaces.cc:2070: Release_Store(reinterpret_cast<AtomicWord*>(address() +
kNextOffset),
What visibility does the Release_Store provide here?

I believe the free list is only manipulated "concurrently" under a lock (in
FreeListCategory::Concatenate), so we should not need synchronization here. (The
same goes for the line below.)

https://codereview.chromium.org/227533006/diff/30001/src/store-buffer.cc
File src/store-buffer.cc (right):

https://codereview.chromium.org/227533006/diff/30001/src/store-buffer.cc#newc...
src/store-buffer.cc:432: HeapObject* object =
reinterpret_cast<HeapObject*>(*slot);
Why could not we get rid of the read of *slot here and use old_value instead?

https://codereview.chromium.org/227533006/diff/30001/src/store-buffer.cc#newc...
src/store-buffer.cc:437: slot_callback(reinterpret_cast<HeapObject**>(slot),
object, old_value);
This extra argument should not be necessary - we could make object = old_value
here.

[Hannes Payer (out of office), 2014-04-08 14:04:35]

https://codereview.chromium.org/227533006/diff/20001/src/spaces.cc
File src/spaces.cc (right):

https://codereview.chromium.org/227533006/diff/20001/src/spaces.cc#newcode2026
src/spaces.cc:2026:
set_map_no_write_barrier(heap->raw_unchecked_free_space_map());
On 2014/04/08 13:00:53, jarin wrote:
> Here we should do unsynchronized write of size and then release write of the
> map, no?
> 
> As it is written here, other threads could still see the new map with a bogus
> size.

Done.

https://codereview.chromium.org/227533006/diff/20001/src/spaces.cc#newcode2070
src/spaces.cc:2070: Release_Store(reinterpret_cast<AtomicWord*>(address() +
kNextOffset),
On 2014/04/08 13:00:53, jarin wrote:
> What visibility does the Release_Store provide here?
> 
> I believe the free list is only manipulated "concurrently" under a lock (in
> FreeListCategory::Concatenate), so we should not need synchronization here.
(The
> same goes for the line below.)

Done. Good point.

https://codereview.chromium.org/227533006/diff/30001/src/store-buffer.cc
File src/store-buffer.cc (right):

https://codereview.chromium.org/227533006/diff/30001/src/store-buffer.cc#newc...
src/store-buffer.cc:432: HeapObject* object =
reinterpret_cast<HeapObject*>(*slot);
On 2014/04/08 13:00:54, jarin wrote:
> Why could not we get rid of the read of *slot here and use old_value instead?

Yes, this one should not be there. I did not upload the latest version. All slot
accessors are going through annotated load/stores.

https://codereview.chromium.org/227533006/diff/30001/src/store-buffer.cc#newc...
src/store-buffer.cc:437: slot_callback(reinterpret_cast<HeapObject**>(slot),
object, old_value);
On 2014/04/08 13:00:54, jarin wrote:
> This extra argument should not be necessary - we could make object = old_value
> here.

Done.
Good point, I was too conservative here.

[Jarin, 2014-04-08 14:44:15]

lgtm. scary stuff.

[Hannes Payer (out of office), 2014-04-08 16:32:11]

Committed patchset #5 manually as r20582 (presubmit successful).