Serializer enable/disable flags need thread safety.

Serializer enable/disable flags need thread safety.

BUG=
R=hpayer@chromium.org, svenpanne@chromium.org

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

[mvstanton, 2014-04-16 09:14:02]

Hi Hannes,
Here is the simplest fix for the 2nd TSAN violation. If performance tests are
okay (in progress), I'd go with this, otherwise I'll craft a different fix per
our discussion.
Thanks,
--Michael

[Sven Panne, 2014-04-16 09:18:46]

DBC

https://codereview.chromium.org/240193002/diff/20001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/240193002/diff/20001/src/arm/assembler-arm.cc...
src/arm/assembler-arm.cc:1082: #ifdef DEBUG
Why do we wrap this into #ifdef DEBUG here plus some other places, but not in
ohters? I propose to move any #ifdef logic (if really necessary) into
TrySetPermanentlyDisabled, avoiding the need to repeat this horror at all call
sites (something which we should do in general).

[mvstanton, 2014-04-16 13:43:29]

Okay, this appears to make TSAN happy(ier), doesn't hurt performance, and
constrains the serializer to make a choice about enabling or disabling at
process start up, which is simpler.

https://codereview.chromium.org/240193002/diff/20001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/240193002/diff/20001/src/arm/assembler-arm.cc...
src/arm/assembler-arm.cc:1082: #ifdef DEBUG
On 2014/04/16 09:18:46, Sven Panne wrote:
> Why do we wrap this into #ifdef DEBUG here plus some other places, but not in
> ohters? I propose to move any #ifdef logic (if really necessary) into
> TrySetPermanentlyDisabled, avoiding the need to repeat this horror at all call
> sites (something which we should do in general).

Thx Sven you got me wondering why we have this call at all. We actually decide
the Enabled/Disabled state at process startup, and we don't toggle it. So this
is superfluous code, removed.

[Sven Panne, 2014-04-17 09:07:34]

lgtm

[Hannes Payer (out of office), 2014-04-17 09:48:10]

https://codereview.chromium.org/240193002/diff/50001/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/240193002/diff/50001/src/serialize.cc#newcode652
src/serialize.cc:652: Atomic32 Serializer::serialization_state_ =
SERIALIZER_STATE_UNINITIALIZED;
AtomicWord

https://codereview.chromium.org/240193002/diff/50001/src/serialize.cc#newcode777
src/serialize.cc:777: NoBarrier_Store(&serialization_state_, state);
Can we have a comment here why this works and why no barrier loads/stores are
fine.
Can there by a enable/disable race?

https://codereview.chromium.org/240193002/diff/50001/src/serialize.cc#newcode791
src/serialize.cc:791: NoBarrier_Store(&serialization_state_,
SERIALIZER_STATE_UNINITIALIZED);
The assumption is that this method is just invoked by one thread, right?
Otherwise it wouldn't work. Please add a comment to explain the global picture.

https://codereview.chromium.org/240193002/diff/50001/src/serialize.h
File src/serialize.h (right):

https://codereview.chromium.org/240193002/diff/50001/src/serialize.h#newcode587
src/serialize.h:587: static Atomic32 serialization_state_;
I would prefer AtomicWord

[mvstanton, 2014-04-17 14:18:29]

Hi Hannes, hi Sven,
Hannes, I address your suggested changes, and Sven, here are the rather tricky
changes required to let ARM generate code in order to probe features for
purposes of printing flags.
Thanks,
--Michael

https://codereview.chromium.org/240193002/diff/50001/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/240193002/diff/50001/src/serialize.cc#newcode652
src/serialize.cc:652: Atomic32 Serializer::serialization_state_ =
SERIALIZER_STATE_UNINITIALIZED;
On 2014/04/17 09:48:10, Hannes Payer wrote:
> AtomicWord

Done.

https://codereview.chromium.org/240193002/diff/50001/src/serialize.cc#newcode777
src/serialize.cc:777: NoBarrier_Store(&serialization_state_, state);
On 2014/04/17 09:48:10, Hannes Payer wrote:
> Can we have a comment here why this works and why no barrier loads/stores are
> fine.
> Can there by a enable/disable race?

Yes, done, and no.

https://codereview.chromium.org/240193002/diff/50001/src/serialize.cc#newcode791
src/serialize.cc:791: NoBarrier_Store(&serialization_state_,
SERIALIZER_STATE_UNINITIALIZED);
On 2014/04/17 09:48:10, Hannes Payer wrote:
> The assumption is that this method is just invoked by one thread, right?
> Otherwise it wouldn't work. Please add a comment to explain the global
picture.

Good point, done.

https://codereview.chromium.org/240193002/diff/50001/src/serialize.h
File src/serialize.h (right):

https://codereview.chromium.org/240193002/diff/50001/src/serialize.h#newcode587
src/serialize.h:587: static Atomic32 serialization_state_;
On 2014/04/17 09:48:10, Hannes Payer wrote:
> I would prefer AtomicWord

Done.

[Hannes Payer (out of office), 2014-04-17 14:23:41]

LGTM from the synchronization side

[mvstanton, 2014-04-17 14:35:18]

Thx, and Sven, I'll check in based on our detailed discussion on the ARM
PrintFlags issue, but as discussed, we'll work to bring more order to
isolate/process initialization.

[mvstanton, 2014-04-17 14:45:31]

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