Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(382)

Issue 199413012: Introduce WTF::releaseStore and WTF::acquireLoad. (Closed)

Created:
6 years, 9 months ago by Vyacheslav Egorov (Chromium)
Modified:
6 years, 9 months ago
CC:
blink-reviews, Mads Ager (chromium), abarth-chromium, Inactive, haraken, Mikhail, kouhei+heap_chromium.org, adamk+blink_chromium.org
Visibility:
Public.

Description

Introduce WTF::releaseStore and WTF::acquireLoad. Use them to avoid data races in the SafePointBarrier discovered by TSAN and guarantee that compiler does not move any memory accesses around m_unparkedThreadCount and m_canResume loads. This change is likely to affect only ARM with its weakly ordered memory model. BUG=347559 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169469

Patch Set 1 #

Total comments: 6

Patch Set 2 : Address glider's comments #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+66 lines, -5 lines) Patch
M Source/heap/ThreadState.cpp View 5 chunks +5 lines, -5 lines 0 comments Download
M Source/wtf/Atomics.h View 1 4 chunks +61 lines, -0 lines 2 comments Download

Messages

Total messages: 18 (0 generated)
Vyacheslav Egorov (Chromium)
Hi Chris, Would you mind taking a look? You seem to touched wtf/Atomics.h last and ...
6 years, 9 months ago (2014-03-14 18:36:33 UTC) #1
Alexander Potapenko
Two style nits, feel free to ignore if they're not applicable to Blink. https://codereview.chromium.org/199413012/diff/1/Source/wtf/Atomics.h File ...
6 years, 9 months ago (2014-03-17 08:02:27 UTC) #2
Dmitry Vyukov
LGTM This is very much appreciated, thanks!
6 years, 9 months ago (2014-03-17 08:49:50 UTC) #3
Vyacheslav Egorov (Chromium)
Adam, could you take a look?
6 years, 9 months ago (2014-03-17 11:57:13 UTC) #4
Vyacheslav Egorov (Chromium)
https://codereview.chromium.org/199413012/diff/1/Source/wtf/Atomics.h File Source/wtf/Atomics.h (right): https://codereview.chromium.org/199413012/diff/1/Source/wtf/Atomics.h#newcode130 Source/wtf/Atomics.h:130: #elif OS(LINUX) || OS(ANDROID) On 2014/03/17 08:02:28, Alexander Potapenko ...
6 years, 9 months ago (2014-03-17 12:00:41 UTC) #5
Alexander Potapenko
https://codereview.chromium.org/199413012/diff/1/Source/wtf/Atomics.h File Source/wtf/Atomics.h (right): https://codereview.chromium.org/199413012/diff/1/Source/wtf/Atomics.h#newcode130 Source/wtf/Atomics.h:130: #elif OS(LINUX) || OS(ANDROID) On 2014/03/17 12:00:41, Vyacheslav Egorov ...
6 years, 9 months ago (2014-03-17 12:09:57 UTC) #6
Vyacheslav Egorov (Chromium)
https://codereview.chromium.org/199413012/diff/1/Source/wtf/Atomics.h File Source/wtf/Atomics.h (right): https://codereview.chromium.org/199413012/diff/1/Source/wtf/Atomics.h#newcode130 Source/wtf/Atomics.h:130: #elif OS(LINUX) || OS(ANDROID) On 2014/03/17 12:09:58, Alexander Potapenko ...
6 years, 9 months ago (2014-03-17 12:16:07 UTC) #7
abarth-chromium
rslgtm This code is kind of crazy...
6 years, 9 months ago (2014-03-18 15:33:30 UTC) #8
abarth-chromium
The CQ bit was checked by abarth@chromium.org
6 years, 9 months ago (2014-03-18 15:33:33 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vegorov@chromium.org/199413012/20001
6 years, 9 months ago (2014-03-18 15:33:43 UTC) #10
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-18 15:38:08 UTC) #11
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
6 years, 9 months ago (2014-03-18 15:38:09 UTC) #12
Vyacheslav Egorov (Chromium)
The CQ bit was checked by vegorov@chromium.org
6 years, 9 months ago (2014-03-18 19:05:20 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vegorov@chromium.org/199413012/20001
6 years, 9 months ago (2014-03-18 19:11:23 UTC) #14
commit-bot: I haz the power
Change committed as 169469
6 years, 9 months ago (2014-03-18 20:17:25 UTC) #15
wibling-chromium
https://codereview.chromium.org/199413012/diff/20001/Source/wtf/Atomics.h File Source/wtf/Atomics.h (right): https://codereview.chromium.org/199413012/diff/20001/Source/wtf/Atomics.h#newcode148 Source/wtf/Atomics.h:148: MEMORY_BARRIER(); I don't quite understand this. Wouldn't you normally ...
6 years, 9 months ago (2014-03-20 08:05:30 UTC) #16
Dmitry Vyukov
https://codereview.chromium.org/199413012/diff/20001/Source/wtf/Atomics.h File Source/wtf/Atomics.h (right): https://codereview.chromium.org/199413012/diff/20001/Source/wtf/Atomics.h#newcode148 Source/wtf/Atomics.h:148: MEMORY_BARRIER(); On 2014/03/20 08:05:31, wibling-chromium wrote: > I don't ...
6 years, 9 months ago (2014-03-20 09:27:48 UTC) #17
wibling
6 years, 9 months ago (2014-03-20 09:36:38 UTC) #18
Thanks. Slava explained it offline as well.



On Thu, Mar 20, 2014 at 10:27 AM, <dvyukov@google.com> wrote:

>
> https://codereview.chromium.org/199413012/diff/20001/Source/wtf/Atomics.h
> File Source/wtf/Atomics.h (right):
>
> https://codereview.chromium.org/199413012/diff/20001/Source/wtf/Atomics.h#
> newcode148
> Source/wtf/Atomics.h:148: MEMORY_BARRIER();
> On 2014/03/20 08:05:31, wibling-chromium wrote:
>
>> I don't quite understand this. Wouldn't you normally put the barrier
>>
> after the
>
>> write and before the read?
>>
>
> No
>
>
>  As it is now can't you have
>>
>
>  Thread0:
>> MB
>> store value
>>
>
>  Thread1:
>> read value (but no barrier so reading local thread cached version)
>> MB
>>
>
> There are no per-thread caches for variables.
>
> https://codereview.chromium.org/199413012/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

Powered by Google App Engine
This is Rietveld 408576698