Switch wtf/SpinLock to std::atomic

Switch wtf/SpinLock to std::atomic

Tsan identified a racy read in the existing spin lock loop.
Tsan is correct in the absolute sense, even though the read is
safe in practice. However, the architectures we support might
change, or the behavior of the compiler might change, so the
the best fix is to just use the proper std::atomic type,
because that is guaranteed to always be safe. And the compiled
code should be effectively unchanged.

BUG=556469
NOTRY=True

Committed: https://crrev.com/5e58730d6fe40ef7cead75954ef5b1c05dea6d4b
Cr-Commit-Position: refs/heads/master@{#363347}

Committed: https://crrev.com/36d01b428979aafa764c28eccc4d73a5e30f112f
Cr-Commit-Position: refs/heads/master@{#363353}

[jschuh, 2015-11-20 07:17:21]

Patchset #4 (id:60001) has been deleted

[jschuh, 2015-12-02 19:52:45]

jschuh@chromium.org changed reviewers:
+ glider@chromium.org, haraken@chromium.org

[jschuh, 2015-12-02 19:52:46]

Turns out it was easy enough to just switch to std::atomic. So, this silences
tsan the right way.

[haraken, 2015-12-03 00:10:39]

haraken@chromium.org changed reviewers:
+ esprehn@chromium.org, yutak@chromium.org

[haraken, 2015-12-03 00:10:39]

I think this change makes sense.

Elliott, yutak: Are you fine with using std::atomic in Blink?

[esprehn, 2015-12-03 01:36:31]

I'd rather we took the code in WebKit, they replaced lots of this stuff
recently. The stuff in std:: turns out to be quite slow.
On Dec 2, 2015 7:10 PM, <haraken@chromium.org> wrote:

> I think this change makes sense.
>
> Elliott, yutak: Are you fine with using std::atomic in Blink?
>
>
> https://codereview.chromium.org/1463683002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Blink Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to blink-reviews+unsubscribe@chromium.org.
>
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2015-12-03 01:36:32]

I'd rather we took the code in WebKit, they replaced lots of this stuff
recently. The stuff in std:: turns out to be quite slow.
On Dec 2, 2015 7:10 PM, <haraken@chromium.org> wrote:

> I think this change makes sense.
>
> Elliott, yutak: Are you fine with using std::atomic in Blink?
>
>
> https://codereview.chromium.org/1463683002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Blink Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to blink-reviews+unsubscribe@chromium.org.
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[haraken, 2015-12-03 02:26:47]

On 2015/12/03 01:36:32, esprehn wrote:
> I'd rather we took the code in WebKit, they replaced lots of this stuff
> recently. The stuff in std:: turns out to be quite slow.

Make a lot of more sense.

[jschuh, 2015-12-03 04:52:27]

Nope. WebKit ripped out their crazy hand-coded atomics and just wrap std::atomic
now. Also, I have no idea what the basis for the "slow" argument was in the
past, but it is definitely not applicable today. I've actually had to hand write
cross-platform atomics before std::atomic, and I can't see anything that would
be a performance concern in the clang intrinsics or std::atomic implementation.

[haraken, 2015-12-03 04:56:03]

On 2015/12/03 04:52:27, jschuh (very slow) wrote:
> Nope. WebKit ripped out their crazy hand-coded atomics and just wrap
std::atomic
> now. Also, I have no idea what the basis for the "slow" argument was in the
> past, but it is definitely not applicable today. I've actually had to hand
write
> cross-platform atomics before std::atomic, and I can't see anything that would
> be a performance concern in the clang intrinsics or std::atomic
implementation.

Is there any link to a discussion/CL of WebKit?

[jschuh, 2015-12-03 05:01:51]

On 2015/12/03 04:56:03, haraken wrote:
> On 2015/12/03 04:52:27, jschuh (very slow) wrote:
> > Nope. WebKit ripped out their crazy hand-coded atomics and just wrap
> std::atomic
> > now. Also, I have no idea what the basis for the "slow" argument was in the
> > past, but it is definitely not applicable today. I've actually had to hand
> write
> > cross-platform atomics before std::atomic, and I can't see anything that
would
> > be a performance concern in the clang intrinsics or std::atomic
> implementation.
> 
> Is there any link to a discussion/CL of WebKit?

Current version: http://trac.webkit.org/browser/trunk/Source/WTF/wtf/Atomics.h
Bug for removing the hand-coded version:
https://bugs.webkit.org/show_bug.cgi?id=149438

[esprehn, 2015-12-03 05:18:47]

WebKit removed SpinLock, they use their own Lock now instead. This patch is just
updating our SpinLock instead of replacing it with the WTF Lock and Condition
which looks much nicer. We should probably put those in base.

I was thinking of

http://trac.webkit.org/changeset/188280
http://trac.webkit.org/log/trunk/Source/WTF/wtf/Lock.h
http://trac.webkit.org/log/trunk/Source/WTF/wtf/Condition.h

and

https://bugs.webkit.org/show_bug.cgi?id=148140

where they replace all std::mutex and std::condition_variable.

std::atomic seems okay though, but we should just get rid of SpinLock itself.

[jschuh, 2015-12-03 06:19:17]

On 2015/12/03 05:18:47, esprehn wrote:
> WebKit removed SpinLock, they use their own Lock now instead. This patch is
just
> updating our SpinLock instead of replacing it with the WTF Lock and Condition
> which looks much nicer. We should probably put those in base.
> 
> I was thinking of
> 
> http://trac.webkit.org/changeset/188280
> http://trac.webkit.org/log/trunk/Source/WTF/wtf/Lock.h
> http://trac.webkit.org/log/trunk/Source/WTF/wtf/Condition.h
> 
> and
> 
> https://bugs.webkit.org/show_bug.cgi?id=148140
> 
> where they replace all std::mutex and std::condition_variable.
> 
> std::atomic seems okay though, but we should just get rid of SpinLock itself.

I am really regretting I ever noticed that the spinlock wasn't yielding, and
decided to do a good deed by fixing it. :(

So, the problem is that these locks are intended for very different use cases.
Our spinlock is a pure spinlock, used only in the allocator, for very short
duration locks. It can't allocate memory inside the lock (like the WebKit Lock's
ParkingLot) and it's POD so it can be statically initialized to the unlocked
state (not the case for the WTF Lock). So the WebKit Lock might be the right
choice for locks that can be held for a long duration, but it doesn't work where
we're using the spinlock.

[Yuta Kitamura, 2015-12-03 06:40:45]

On 2015/12/03 06:19:17, jschuh (very slow) wrote:
> So, the problem is that these locks are intended for very different use cases.
> Our spinlock is a pure spinlock, used only in the allocator, for very short
> duration locks. It can't allocate memory inside the lock (like the WebKit
Lock's
> ParkingLot) and it's POD so it can be statically initialized to the unlocked
> state (not the case for the WTF Lock). So the WebKit Lock might be the right
> choice for locks that can be held for a long duration, but it doesn't work
where
> we're using the spinlock.

I think this argument makes a lot of sense and we could keep SpinLock
for these use cases (only).

What kind of TSAN warning is this patch trying to fix? Why is std::atomics
needed? Could you elaborate your change description to explain these points?

[jschuh, 2015-12-03 17:37:46]

On 2015/12/03 06:40:45, Yuta Kitamura wrote:
> On 2015/12/03 06:19:17, jschuh (very slow) wrote:
> > So, the problem is that these locks are intended for very different use
cases.
> > Our spinlock is a pure spinlock, used only in the allocator, for very short
> > duration locks. It can't allocate memory inside the lock (like the WebKit
> Lock's
> > ParkingLot) and it's POD so it can be statically initialized to the unlocked
> > state (not the case for the WTF Lock). So the WebKit Lock might be the right
> > choice for locks that can be held for a long duration, but it doesn't work
> where
> > we're using the spinlock.

One thing I forgot to mention is that if you extract the ParkingLot from the
WebKit Lock, you get basically the same yielding spinlock that we're using here.
 
> I think this argument makes a lot of sense and we could keep SpinLock
> for these use cases (only).
> 
> What kind of TSAN warning is this patch trying to fix?

Tsan is identifying a racey read in the spin lock loop. Tsan is correct in the
absolute sense, but the read is word aligned and atomic on all architectures we
support, so it's safe in practice. However, the architectures we support might
change, or the behavior of the compiler might change, so it's best to just fix
it permanently rather than add an exception.

> Why is std::atomics needed? 

It's not needed so much as it is the future-proof way of performing these
operations. Logically, the std::atomics are going to compile down to almost
exactly the same binary as the code compiles to today. However, by using
std::atomics we guarantee that changes in architectures or the compiler will not
break it going forward.

> Could you elaborate your change description to explain these points?

All the context is explained in the linked bug, and it seems a bit too long and
involved to put that much depth in the CL description. At least, my personal
preference is to keep CL descriptions simple and let the bug provide the full
detail.

[esprehn, 2015-12-03 19:02:50]

In blink we prefer change descriptions that say why you're doing something, so
you don't have to go read a big thread in the bug to understand the change.
Especially given that the bug you linked is actually restricted so not everyone
can see it.

Please explain the data race and how this fixes it in the change description and
why this change makes sense.

lgtm with that fixed.

[jschuh, 2015-12-03 19:17:23]

Description was changed from

==========
Switch wtf/SpinLock to std::atomic

This eliminates tsan warnings and makes the code correct for C++11.

BUG=556469
==========

to

==========
Switch wtf/SpinLock to std::atomic

Tsan identified a racy read in the existing spin lock loop.
Tsan is correct in the absolute sense, even though the read is
safe in practice. However, the architectures we support might
change, or the behavior of the compiler might change, so the
the best fix is to just use the proper std::atomic type,
because that is guaranteed to always be safe. And the compiled
code should be effectively unchanged.

BUG=556469
==========

[jschuh, 2015-12-03 19:19:20]

On 2015/12/03 19:02:50, esprehn wrote:
> In blink we prefer change descriptions that say why you're doing something, so
> you don't have to go read a big thread in the bug to understand the change.
> Especially given that the bug you linked is actually restricted so not
everyone
> can see it.
> 
> Please explain the data race and how this fixes it in the change description
and
> why this change makes sense.
> 
> lgtm with that fixed.

Done.

[jschuh, 2015-12-03 19:19:39]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2015-12-03 19:22:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463683002/100002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463683002/100002

[commit-bot: I haz the power, 2015-12-03 19:42:43]

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

[commit-bot: I haz the power, 2015-12-03 19:42:44]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[jschuh, 2015-12-03 19:54:49]

Oops, still need sign off from haraken.

[haraken, 2015-12-03 23:26:59]

LGTM, though you need to address some compile issues in bots.

[Yuta Kitamura, 2015-12-04 03:36:28]

LGTM as well.

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/SpinLock.h (right):

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/SpinLock.h:68: WTF_EXPORT void SlowLock();
Follow Blink's naming convention.

[Alexander Potapenko, 2015-12-04 12:17:34]

When the memory ordering is not specified, std::atomic operations default to
memory_order_seq_cst, which is way too strong for your case (and may probably
kill some of the performance).
I think it's better to specify the ordering explicitly.

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/PartitionAlloc.h (right):

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/PartitionAlloc.h:791: SpinLock::Guard
guard(root->lock);
How about putting this code in a nested scope? If someone decides to add more
code below this will emphasize the scope of the lock.

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/SpinLock.cpp (right):

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/SpinLock.cpp:67: if (!m_lock.load() &&
LIKELY(!m_lock.exchange(true)))
load() can be made memory_order_relaxed, and exchange() - memory_order_acq_rel.

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/SpinLock.cpp:73: } while (m_lock.exchange(true));
memory_order_acq_rel

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/SpinLock.cpp:74: } while
(UNLIKELY(m_lock.exchange(true)));
ditto.

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/SpinLock.h (right):

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/SpinLock.h:55: if (LIKELY(!m_lock.exchange(true)))
I suppose it's enough to have memory_order_acq_rel here.

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/SpinLock.h:62: m_lock.store(false);
Should be enough to store with memory_order_release.

[jschuh, 2015-12-04 20:44:06]

Patchset #8 (id:150001) has been deleted

[jschuh, 2015-12-04 20:45:14]

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/PartitionAlloc.h (right):

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/PartitionAlloc.h:791: SpinLock::Guard
guard(root->lock);
On 2015/12/04 12:17:34, Alexander Potapenko wrote:
> How about putting this code in a nested scope? If someone decides to add more
> code below this will emphasize the scope of the lock.

Done.

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/SpinLock.cpp (right):

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/SpinLock.cpp:67: if (!m_lock.load() &&
LIKELY(!m_lock.exchange(true)))
On 2015/12/04 12:17:34, Alexander Potapenko wrote:
> load() can be made memory_order_relaxed, and exchange() -
memory_order_acq_rel.

Done.

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/SpinLock.cpp:73: } while (m_lock.exchange(true));
On 2015/12/04 12:17:34, Alexander Potapenko wrote:
> memory_order_acq_rel

Actually, this was a mistake on my part and should just be a load with
memory_order_relaxed.

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/SpinLock.cpp:74: } while
(UNLIKELY(m_lock.exchange(true)));
On 2015/12/04 12:17:34, Alexander Potapenko wrote:
> ditto.

Done.

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/SpinLock.h (right):

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/SpinLock.h:55: if (LIKELY(!m_lock.exchange(true)))
On 2015/12/04 12:17:34, Alexander Potapenko wrote:
> I suppose it's enough to have memory_order_acq_rel here.

Done.

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/SpinLock.h:62: m_lock.store(false);
On 2015/12/04 12:17:34, Alexander Potapenko wrote:
> Should be enough to store with memory_order_release.

Done.

https://codereview.chromium.org/1463683002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/SpinLock.h:68: WTF_EXPORT void SlowLock();
On 2015/12/04 03:36:28, Yuta Kitamura wrote:
> Follow Blink's naming convention.

Done.

[jschuh, 2015-12-04 20:45:20]

The CQ bit was checked by jschuh@chromium.org

[jschuh, 2015-12-04 20:45:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, esprehn@chromium.org, yutak@chromium.org
Link to the patchset: https://codereview.chromium.org/1463683002/#ps170001
(title: "actioned feedback")

[commit-bot: I haz the power, 2015-12-04 20:46:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463683002/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463683002/170001

[commit-bot: I haz the power, 2015-12-04 21:14:01]

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

[commit-bot: I haz the power, 2015-12-04 21:14:02]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[jschuh, 2015-12-05 00:55:46]

The CQ bit was checked by jschuh@chromium.org

[jschuh, 2015-12-05 00:55:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, esprehn@chromium.org, yutak@chromium.org
Link to the patchset: https://codereview.chromium.org/1463683002/#ps190001
(title: "iwyu fix")

[commit-bot: I haz the power, 2015-12-05 00:56:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463683002/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463683002/190001

[commit-bot: I haz the power, 2015-12-05 03:01:00]

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

[commit-bot: I haz the power, 2015-12-05 03:01:01]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[jschuh, 2015-12-05 03:02:48]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2015-12-05 03:07:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463683002/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463683002/190001

[commit-bot: I haz the power, 2015-12-05 05:10:18]

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

[commit-bot: I haz the power, 2015-12-05 05:10:20]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[jschuh, 2015-12-05 06:35:13]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2015-12-05 06:36:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463683002/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463683002/190001

[Alexander Potapenko, 2015-12-05 06:54:19]

On 2015/12/05 06:36:01, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1463683002/190001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1463683002/190001

Justin, can you please also run the tsan_chromium trybot?

[commit-bot: I haz the power, 2015-12-05 08:38:11]

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

[commit-bot: I haz the power, 2015-12-05 08:38:12]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[jschuh, 2015-12-05 19:36:15]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2015-12-05 19:36:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463683002/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463683002/190001

[jschuh, 2015-12-05 21:28:08]

On 2015/12/05 06:54:19, Alexander Potapenko wrote:
> On 2015/12/05 06:36:01, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1463683002/190001
> > View timeline at
> >  https://chromium-cq-status.appspot.com/patch-timeline/1463683002/190001
> 
> Justin, can you please also run the tsan_chromium trybot?

I've been doing that, so I gave it another run and it seems to be fine (modulo
some spurious flake).

[jschuh, 2015-12-05 21:29:59]

The Windows bots haven't responded for over a day and I already verified the
patch locally on Windows. Now that the ICWYU issue is fixed and all the other
bots are green, I'm just going to push it through with a NOTRY.

[jschuh, 2015-12-05 21:30:37]

Description was changed from

==========
Switch wtf/SpinLock to std::atomic

Tsan identified a racy read in the existing spin lock loop.
Tsan is correct in the absolute sense, even though the read is
safe in practice. However, the architectures we support might
change, or the behavior of the compiler might change, so the
the best fix is to just use the proper std::atomic type,
because that is guaranteed to always be safe. And the compiled
code should be effectively unchanged.

BUG=556469
==========

to

==========
Switch wtf/SpinLock to std::atomic

Tsan identified a racy read in the existing spin lock loop.
Tsan is correct in the absolute sense, even though the read is
safe in practice. However, the architectures we support might
change, or the behavior of the compiler might change, so the
the best fix is to just use the proper std::atomic type,
because that is guaranteed to always be safe. And the compiled
code should be effectively unchanged.

BUG=556469
NOTRY=True
==========

[jschuh, 2015-12-05 21:30:53]

The CQ bit was unchecked by jschuh@chromium.org

[jschuh, 2015-12-05 21:31:02]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2015-12-05 21:33:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463683002/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463683002/190001

[commit-bot: I haz the power, 2015-12-05 21:41:10]

Description was changed from

==========
Switch wtf/SpinLock to std::atomic

Tsan identified a racy read in the existing spin lock loop.
Tsan is correct in the absolute sense, even though the read is
safe in practice. However, the architectures we support might
change, or the behavior of the compiler might change, so the
the best fix is to just use the proper std::atomic type,
because that is guaranteed to always be safe. And the compiled
code should be effectively unchanged.

BUG=556469
NOTRY=True
==========

to

==========
Switch wtf/SpinLock to std::atomic

Tsan identified a racy read in the existing spin lock loop.
Tsan is correct in the absolute sense, even though the read is
safe in practice. However, the architectures we support might
change, or the behavior of the compiler might change, so the
the best fix is to just use the proper std::atomic type,
because that is guaranteed to always be safe. And the compiled
code should be effectively unchanged.

BUG=556469
NOTRY=True
==========

[commit-bot: I haz the power, 2015-12-05 21:41:11]

Committed patchset #9 (id:190001)

[commit-bot: I haz the power, 2015-12-05 21:42:12]

Description was changed from

==========
Switch wtf/SpinLock to std::atomic

Tsan identified a racy read in the existing spin lock loop.
Tsan is correct in the absolute sense, even though the read is
safe in practice. However, the architectures we support might
change, or the behavior of the compiler might change, so the
the best fix is to just use the proper std::atomic type,
because that is guaranteed to always be safe. And the compiled
code should be effectively unchanged.

BUG=556469
NOTRY=True
==========

to

==========
Switch wtf/SpinLock to std::atomic

Tsan identified a racy read in the existing spin lock loop.
Tsan is correct in the absolute sense, even though the read is
safe in practice. However, the architectures we support might
change, or the behavior of the compiler might change, so the
the best fix is to just use the proper std::atomic type,
because that is guaranteed to always be safe. And the compiled
code should be effectively unchanged.

BUG=556469
NOTRY=True

Committed: https://crrev.com/5e58730d6fe40ef7cead75954ef5b1c05dea6d4b
Cr-Commit-Position: refs/heads/master@{#363347}
==========

[commit-bot: I haz the power, 2015-12-05 21:42:13]

Patchset 9 (id:??) landed as
https://crrev.com/5e58730d6fe40ef7cead75954ef5b1c05dea6d4b
Cr-Commit-Position: refs/heads/master@{#363347}

[jschuh, 2015-12-05 22:49:13]

A revert of this CL (patchset #9 id:190001) has been created in
https://codereview.chromium.org/1502023002/ by jschuh@chromium.org.

The reason for reverting is: Tree is closed (Automatic: "compile" on
http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20...
"Linux Chromium OS ASan LSan Builder" from
5e58730d6fe40ef7cead75954ef5b1c05dea6d4b : jschuh )
.

[jschuh, 2015-12-05 23:07:55]

On 2015/12/05 22:49:13, jschuh (very slow) wrote:
> A revert of this CL (patchset #9 id:190001) has been created in
> https://codereview.chromium.org/1502023002/ by mailto:jschuh@chromium.org.
> 
> The reason for reverting is: Tree is closed (Automatic: "compile" on
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20...
> "Linux Chromium OS ASan LSan Builder" from
> 5e58730d6fe40ef7cead75954ef5b1c05dea6d4b : jschuh )
> .

More include-what-you-use compile failures, but this time for a waterfall bot
that apparently doesn't have CQ coverage. Awesome.

On the plus side, it looks like it's just a missing include in wtf/Vector.h, and
it would have stuck otherwise. Unfortunately, now I need to figure out how to
test that broken configuration.

[jschuh, 2015-12-05 23:26:40]

On 2015/12/05 23:07:55, jschuh (very slow) wrote:
> On 2015/12/05 22:49:13, jschuh (very slow) wrote:
> > A revert of this CL (patchset #9 id:190001) has been created in
> > https://codereview.chromium.org/1502023002/ by mailto:jschuh@chromium.org.
> > 
> > The reason for reverting is: Tree is closed (Automatic: "compile" on
> >
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20...
> > "Linux Chromium OS ASan LSan Builder" from
> > 5e58730d6fe40ef7cead75954ef5b1c05dea6d4b : jschuh )
> > .
> 
> More include-what-you-use compile failures, but this time for a waterfall bot
> that apparently doesn't have CQ coverage. Awesome.
> 
> On the plus side, it looks like it's just a missing include in wtf/Vector.h,
and
> it would have stuck otherwise. Unfortunately, now I need to figure out how to
> test that broken configuration.

Oh, wtf/ContainerAnnotations.h should be including wtf/AddressSanitizer.h. I
should just be able to test that by running a try against the normal ASAN bot.

[jschuh, 2015-12-05 23:27:09]

Description was changed from

==========
Switch wtf/SpinLock to std::atomic

Tsan identified a racy read in the existing spin lock loop.
Tsan is correct in the absolute sense, even though the read is
safe in practice. However, the architectures we support might
change, or the behavior of the compiler might change, so the
the best fix is to just use the proper std::atomic type,
because that is guaranteed to always be safe. And the compiled
code should be effectively unchanged.

BUG=556469
NOTRY=True

Committed: https://crrev.com/5e58730d6fe40ef7cead75954ef5b1c05dea6d4b
Cr-Commit-Position: refs/heads/master@{#363347}
==========

to

==========
Switch wtf/SpinLock to std::atomic

Tsan identified a racy read in the existing spin lock loop.
Tsan is correct in the absolute sense, even though the read is
safe in practice. However, the architectures we support might
change, or the behavior of the compiler might change, so the
the best fix is to just use the proper std::atomic type,
because that is guaranteed to always be safe. And the compiled
code should be effectively unchanged.

BUG=556469
NOTRY=True

Committed: https://crrev.com/5e58730d6fe40ef7cead75954ef5b1c05dea6d4b
Cr-Commit-Position: refs/heads/master@{#363347}
==========

[jschuh, 2015-12-06 03:48:12]

The CQ bit was checked by jschuh@chromium.org

[jschuh, 2015-12-06 03:48:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, esprehn@chromium.org, yutak@chromium.org
Link to the patchset: https://codereview.chromium.org/1463683002/#ps210001
(title: "asan iwyu")

[commit-bot: I haz the power, 2015-12-06 03:48:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463683002/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463683002/210001

[commit-bot: I haz the power, 2015-12-06 04:27:58]

Description was changed from

==========
Switch wtf/SpinLock to std::atomic

Tsan identified a racy read in the existing spin lock loop.
Tsan is correct in the absolute sense, even though the read is
safe in practice. However, the architectures we support might
change, or the behavior of the compiler might change, so the
the best fix is to just use the proper std::atomic type,
because that is guaranteed to always be safe. And the compiled
code should be effectively unchanged.

BUG=556469
NOTRY=True

Committed: https://crrev.com/5e58730d6fe40ef7cead75954ef5b1c05dea6d4b
Cr-Commit-Position: refs/heads/master@{#363347}
==========

to

==========
Switch wtf/SpinLock to std::atomic

Tsan identified a racy read in the existing spin lock loop.
Tsan is correct in the absolute sense, even though the read is
safe in practice. However, the architectures we support might
change, or the behavior of the compiler might change, so the
the best fix is to just use the proper std::atomic type,
because that is guaranteed to always be safe. And the compiled
code should be effectively unchanged.

BUG=556469
NOTRY=True

Committed: https://crrev.com/5e58730d6fe40ef7cead75954ef5b1c05dea6d4b
Cr-Commit-Position: refs/heads/master@{#363347}
==========

[commit-bot: I haz the power, 2015-12-06 04:27:59]

Committed patchset #10 (id:210001)

[commit-bot: I haz the power, 2015-12-06 04:28:45]

Description was changed from

==========
Switch wtf/SpinLock to std::atomic

Tsan identified a racy read in the existing spin lock loop.
Tsan is correct in the absolute sense, even though the read is
safe in practice. However, the architectures we support might
change, or the behavior of the compiler might change, so the
the best fix is to just use the proper std::atomic type,
because that is guaranteed to always be safe. And the compiled
code should be effectively unchanged.

BUG=556469
NOTRY=True

Committed: https://crrev.com/5e58730d6fe40ef7cead75954ef5b1c05dea6d4b
Cr-Commit-Position: refs/heads/master@{#363347}
==========

to

==========
Switch wtf/SpinLock to std::atomic

Tsan identified a racy read in the existing spin lock loop.
Tsan is correct in the absolute sense, even though the read is
safe in practice. However, the architectures we support might
change, or the behavior of the compiler might change, so the
the best fix is to just use the proper std::atomic type,
because that is guaranteed to always be safe. And the compiled
code should be effectively unchanged.

BUG=556469
NOTRY=True

Committed: https://crrev.com/5e58730d6fe40ef7cead75954ef5b1c05dea6d4b
Cr-Commit-Position: refs/heads/master@{#363347}

Committed: https://crrev.com/36d01b428979aafa764c28eccc4d73a5e30f112f
Cr-Commit-Position: refs/heads/master@{#363353}
==========

[commit-bot: I haz the power, 2015-12-06 04:28:46]

Patchset 10 (id:??) landed as
https://crrev.com/36d01b428979aafa764c28eccc4d73a5e30f112f
Cr-Commit-Position: refs/heads/master@{#363353}

[Primiano Tucci (use gerrit), 2015-12-07 10:11:56]

primiano@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-12-07 10:11:57]

Static initializer :-/

https://codereview.chromium.org/1463683002/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/Partitions.cpp (right):

https://codereview.chromium.org/1463683002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/Partitions.cpp:42: SpinLock
Partitions::s_initializationLock;
This is now a static initializer, some bots are unhappy.
See:
http://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/4693/step...

FAILED: ninja -t msvc -e environment.x86 --
"..\..\third_party/llvm-build/Release+Asserts/bin/clang-cl" -m32 /nologo
/showIncludes /FC @obj\third_party\WebKit\Source\wtf\wtf.Partitions.obj.rsp /c
..\..\third_party\WebKit\Source\wtf\Partitions.cpp
/Foobj\third_party\WebKit\Source\wtf\wtf.Partitions.obj
/Fdobj\third_party\WebKit\Source\wtf\wtf.cc.pdb 
..\..\third_party\WebKit\Source\wtf\Partitions.cpp(42,22) :  error: declaration
requires a global constructor [-Werror,-Wglobal-constructors]
SpinLock Partitions::s_initializationLock;
                     ^~~~~~~~~~~~~~~~~~~~

[sof, 2015-12-07 10:13:24]

On 2015/12/07 10:11:57, Primiano Tucci wrote:
> Static initializer :-/
> 
>
https://codereview.chromium.org/1463683002/diff/210001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/wtf/Partitions.cpp (right):
> 
>
https://codereview.chromium.org/1463683002/diff/210001/third_party/WebKit/Sou...
> third_party/WebKit/Source/wtf/Partitions.cpp:42: SpinLock
> Partitions::s_initializationLock;
> This is now a static initializer, some bots are unhappy.
> See:
>
http://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/4693/step...
> 
> FAILED: ninja -t msvc -e environment.x86 --
> "..\..\third_party/llvm-build/Release+Asserts/bin/clang-cl" -m32 /nologo
> /showIncludes /FC @obj\third_party\WebKit\Source\wtf\wtf.Partitions.obj.rsp /c
> ..\..\third_party\WebKit\Source\wtf\Partitions.cpp
> /Foobj\third_party\WebKit\Source\wtf\wtf.Partitions.obj
> /Fdobj\third_party\WebKit\Source\wtf\wtf.cc.pdb 
> ..\..\third_party\WebKit\Source\wtf\Partitions.cpp(42,22) :  error:
declaration
> requires a global constructor [-Werror,-Wglobal-constructors]
> SpinLock Partitions::s_initializationLock;
>                      ^~~~~~~~~~~~~~~~~~~~

Issue tracked by https://code.google.com/p/chromium/issues/detail?id=566751