New issue
Advanced search Search tips

Issue 736037 link

Starred by 2 users
Status: Fixed
Owner:
jbroman@chromium.org
Closed: Dec 11
Cc:
yutak@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Atomic ref counts (base, wtf) can be built on std::atomic

Project Member Reported by jbroman@chromium.org, Jun 22 2017 
This is the more modern/idiomatic approach, is now permitted in these areas, and allows access to the ref count to be controlled (so that only operations that make sense on a reference count can occur).
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 27 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/51737d986a64e757570a5f0062863bbcad2e35ca

commit 51737d986a64e757570a5f0062863bbcad2e35ca
Author: Jeremy Roman <jbroman@chromium.org>
Date: Tue Jun 27 19:01:00 2017

Make gpu::PreemptionFlag use std::atomic_int.

It presently uses a combination of atomic ref counting ops and subtle atomic
ops, which is strange and breaks the invariants of base::AtomicRefCount.

Acquire/release semantics are used, so that reading the pre-emption flag
implies that the changes that were made to memory before it was set or reset
to its visible value will also be visible.

Bug:  736037 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I7435d673298c9d2d46f41f8675ae70a9dd75cdee
Reviewed-on: https://chromium-review.googlesource.com/544319
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482700}
[modify] https://crrev.com/51737d986a64e757570a5f0062863bbcad2e35ca/gpu/command_buffer/service/preemption_flag.h
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 29 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/031d45a1058ecf90d45c25b550734b5bf9b608dd

commit 031d45a1058ecf90d45c25b550734b5bf9b608dd
Author: Jeremy Roman <jbroman@chromium.org>
Date: Thu Jun 29 01:01:24 2017

Implement base::AtomicRefCount as a class wrapping std::atomic_int.

This is the more standard approach since C++11, and it allows this class to
be encapsulated (preventing abuse by performing other operations on the data).

If landed, call sites will be rewritten to call member functions directly.

A warning must be suppressed in MSVC, because std::atomic forces alignment,
and MSVC warns on end-padding due to alignment on a class, which occurs
in base::RefCountedThreadSafe in debug builds (and the warning is not
targeted at base/memory/ref_counted.*, so it's hard to suppress).

Bug:  736037 
Tbr: rockot@chromium.org,xiyuan@chromium.org
Change-Id: Ieb0599b4a67a4e8db06bd4d08c3126baea9207e2
Reviewed-on: https://chromium-review.googlesource.com/550378
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Taiju Tsuiki <tzik@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483238}
[modify] https://crrev.com/031d45a1058ecf90d45c25b550734b5bf9b608dd/base/atomic_ref_count.h
[modify] https://crrev.com/031d45a1058ecf90d45c25b550734b5bf9b608dd/base/memory/ref_counted.cc
[modify] https://crrev.com/031d45a1058ecf90d45c25b550734b5bf9b608dd/base/memory/ref_counted.h
[modify] https://crrev.com/031d45a1058ecf90d45c25b550734b5bf9b608dd/build/config/compiler/BUILD.gn
[modify] https://crrev.com/031d45a1058ecf90d45c25b550734b5bf9b608dd/chrome/browser/chromeos/login/signin/merge_session_throttling_utils.cc
[modify] https://crrev.com/031d45a1058ecf90d45c25b550734b5bf9b608dd/device/base/synchronization/one_writer_seqlock_unittest.cc
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 29 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7cab7ac9f9bd90e440812fe5f4266ebd4b73b3b9

commit 7cab7ac9f9bd90e440812fe5f4266ebd4b73b3b9
Author: Jeremy Roman <jbroman@chromium.org>
Date: Thu Jun 29 21:24:02 2017

Build WTF::ThreadSafeRefCounted using base::AtomicRefCount.

It is now built on std::atomic, using acquire/release semantics.
This reduces the code needed in WTF, and improves the generated
code on ARM.

Bug:  736037 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I3e3d1725e81d4f809df2b1cdf2c3def7847daac1
Reviewed-on: https://chromium-review.googlesource.com/521822
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Yuta Kitamura <yutak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483490}
[modify] https://crrev.com/7cab7ac9f9bd90e440812fe5f4266ebd4b73b3b9/third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp
[modify] https://crrev.com/7cab7ac9f9bd90e440812fe5f4266ebd4b73b3b9/third_party/WebKit/Source/platform/wtf/DEPS
[modify] https://crrev.com/7cab7ac9f9bd90e440812fe5f4266ebd4b73b3b9/third_party/WebKit/Source/platform/wtf/ThreadSafeRefCounted.h
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 6 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ee96d561c0b6d4933f94ef181e8e0e8cc03b2d62

commit ee96d561c0b6d4933f94ef181e8e0e8cc03b2d62
Author: Jeremy Roman <jbroman@chromium.org>
Date: Thu Jul 06 19:09:36 2017

Replace base::AtomicRefCount functions with member functions.

Also, the use of AtomicRefCount to check for cross-thread use of non-atomic
RefCounted is suspect (the guarantees provided by AtomicRefCount aren't
obviously strong enough for this use). Since this is only used for DCHECKs,
it seems simplest to have it use sequentially-consistent atomics and not
think about it further.

Bug:  736037 
Tbr: sky@chromium.org
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I1613e5f39fa814ceef3cec5bd25806179b65c978
Reviewed-on: https://chromium-review.googlesource.com/556014
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Taiju Tsuiki <tzik@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484700}
[modify] https://crrev.com/ee96d561c0b6d4933f94ef181e8e0e8cc03b2d62/base/atomic_ref_count.h
[modify] https://crrev.com/ee96d561c0b6d4933f94ef181e8e0e8cc03b2d62/base/barrier_closure.cc
[modify] https://crrev.com/ee96d561c0b6d4933f94ef181e8e0e8cc03b2d62/base/memory/ref_counted.cc
[modify] https://crrev.com/ee96d561c0b6d4933f94ef181e8e0e8cc03b2d62/base/memory/ref_counted.h
[modify] https://crrev.com/ee96d561c0b6d4933f94ef181e8e0e8cc03b2d62/base/win/iunknown_impl.cc
[modify] https://crrev.com/ee96d561c0b6d4933f94ef181e8e0e8cc03b2d62/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/ee96d561c0b6d4933f94ef181e8e0e8cc03b2d62/chrome/browser/chromeos/login/signin/merge_session_throttling_utils.cc
[modify] https://crrev.com/ee96d561c0b6d4933f94ef181e8e0e8cc03b2d62/device/base/synchronization/one_writer_seqlock_unittest.cc
[modify] https://crrev.com/ee96d561c0b6d4933f94ef181e8e0e8cc03b2d62/media/audio/audio_output_controller.cc
[modify] https://crrev.com/ee96d561c0b6d4933f94ef181e8e0e8cc03b2d62/tools/gn/scheduler.cc

Comment 5 by benhenry@google.com, Aug 23

Components: -Internals
Project Member

Comment 8 by bugdroid1@chromium.org, Nov 27 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6c181a470d3769efaa2dab92b5bcb31344bca6ef

commit 6c181a470d3769efaa2dab92b5bcb31344bca6ef
Author: Jeremy Roman <jbroman@chromium.org>
Date: Tue Nov 27 14:21:34 2018

Use <atomic> in text_encoding_registry.cc.

Bug:  736037 
Change-Id: I4b4b0e221ac987f0c2de81cbc2709c7711d0d1df
Reviewed-on: https://chromium-review.googlesource.com/c/1351450
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611098}
[modify] https://crrev.com/6c181a470d3769efaa2dab92b5bcb31344bca6ef/third_party/blink/renderer/platform/wtf/text/text_encoding_registry.cc
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 28 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1a7558e3b3b7767237d4398f8bab6e761ce4cd56

commit 1a7558e3b3b7767237d4398f8bab6e761ce4cd56
Author: Jeremy Roman <jbroman@chromium.org>
Date: Wed Nov 28 01:17:46 2018

Remove blink::PersistentBase::AtomicGet.

Writes to cross-thread persistents are no longer atomic (release) stores, so
this is not more safe than a non-atomic access -- its correctness depends on
external synchronization, which at the sole call site is provided by
ProcessHeap::CrossThreadPersistentMutex(), which also guards all writes to
PersistentBase::raw_ for cross-thread handles.

Bug:  736037 
Change-Id: Ic47ef65b731030cdf9897f2d7dd48899514eea5e
Reviewed-on: https://chromium-review.googlesource.com/c/1351567
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611486}
[modify] https://crrev.com/1a7558e3b3b7767237d4398f8bab6e761ce4cd56/third_party/blink/renderer/platform/heap/persistent.h
[modify] https://crrev.com/1a7558e3b3b7767237d4398f8bab6e761ce4cd56/third_party/blink/renderer/platform/heap/persistent_node.cc
Project Member

Comment 11 by bugdroid1@chromium.org, Nov 29 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5ffb101168071a92ce07bef4ee98608537198799

commit 5ffb101168071a92ce07bef4ee98608537198799
Author: Jeremy Roman <jbroman@chromium.org>
Date: Thu Nov 29 05:40:35 2018

Use std::atomic for WTF hash table stats.

Ordering guarantees required here are very weak, as it merely gathers aggregate
stats about hash table use and external synchronization is expected if precise
results are required.

Did a quick local test to confirm that it still works correctly.

Bug:  736037 
Change-Id: Icc780ad38cd62474f2c21a973dfc393d5aa1ab75
Reviewed-on: https://chromium-review.googlesource.com/c/1352577
Commit-Queue: Yuta Kitamura <yutak@chromium.org>
Reviewed-by: Yuta Kitamura <yutak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612059}
[modify] https://crrev.com/5ffb101168071a92ce07bef4ee98608537198799/third_party/blink/renderer/platform/wtf/hash_table.cc
[modify] https://crrev.com/5ffb101168071a92ce07bef4ee98608537198799/third_party/blink/renderer/platform/wtf/hash_table.h
Project Member

Comment 12 by bugdroid1@chromium.org, Dec 6 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/12ea27e64fc74784c517bc88488503d064204143

commit 12ea27e64fc74784c517bc88488503d064204143
Author: Jeremy Roman <jbroman@chromium.org>
Date: Thu Dec 06 14:43:24 2018

Refactor the "is incremental marking" counter into an "entry flag" class.

1. Switch to <atomic>, which is the future of atomics in Blink.

2. Since this logic is subtle, it is factored out into a separate class
   with a verbose comment, explaining why its semantics are correct.

3. Since the load didn't previously synchronize with the store, and the
   only requirement a thread has is that it sees its own writes (and the
   data doesn't become corrupt), std::memory_order_relaxed is used for
   all accesses.

Bug:  736037 
Change-Id: I017da1ebfba3e95e5d0393014dc57fb4958bd6b1
Reviewed-on: https://chromium-review.googlesource.com/c/1364151
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614340}
[modify] https://crrev.com/12ea27e64fc74784c517bc88488503d064204143/third_party/blink/renderer/platform/heap/BUILD.gn
[add] https://crrev.com/12ea27e64fc74784c517bc88488503d064204143/third_party/blink/renderer/platform/heap/atomic_entry_flag.h
[modify] https://crrev.com/12ea27e64fc74784c517bc88488503d064204143/third_party/blink/renderer/platform/heap/thread_state.cc
[modify] https://crrev.com/12ea27e64fc74784c517bc88488503d064204143/third_party/blink/renderer/platform/heap/thread_state.h
Project Member

Comment 13 by bugdroid1@chromium.org, Dec 6 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/63ff3ddea380122470a6633bb72be00c1b153e63

commit 63ff3ddea380122470a6633bb72be00c1b153e63
Author: Jeremy Roman <jbroman@chromium.org>
Date: Thu Dec 06 16:12:39 2018

Move blink::InstanceCounters to use relaxed std::atomic, and add a warning comment.

The comment is added because it is clearly tempting to rely on these counters
to provide an accurate count of the number of instances that exist across threads,
but that is not something that can be computed with atomics (which do not provide
mutual exclusion).

It doesn't appear that these atomics intend to synchronize any other memory access
(and in fact in many cases are used only from a single thread).

Bug:  736037 
Change-Id: I2372ae73878361024779aadb39a72983f4faa3bb
Reviewed-on: https://chromium-review.googlesource.com/c/1355885
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614371}
[modify] https://crrev.com/63ff3ddea380122470a6633bb72be00c1b153e63/third_party/blink/renderer/platform/instance_counters.cc
[modify] https://crrev.com/63ff3ddea380122470a6633bb72be00c1b153e63/third_party/blink/renderer/platform/instance_counters.h
Project Member

Comment 15 by bugdroid1@chromium.org, Dec 7 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/13f12c8d2783426248a4d5f77681c0947bc46b48

commit 13f12c8d2783426248a4d5f77681c0947bc46b48
Author: Jeremy Roman <jbroman@chromium.org>
Date: Fri Dec 07 22:57:51 2018

heap: Use standard C++ <atomic> to implement cross-thread pointers to PersistentNode.

Since this causes the type of the pointer for this case to change from
PersistentNode* to std::atomic<PersistentNode*>, this makes the strategy of
choosing whether to do atomic operations or not using an |if| statement invalid,
as that requires that the code in both branches be *valid* (even if it would get
optimized out later) -- since we don't have C++17's constexpr if.

As a result, those parts of PersistentBase which manipulate this pointer in that
way are moved into a member which encapsulates that logic -- PersistentNodePtr for
the single-thread case, and CrossThreadPersistentNodePtr for the cross-thread case.
These expose a compatible interface and prevent this compiler error.

Bug:  736037 
Change-Id: I00ba2fb76e95d1c00be3931990361ffb374fc231
Reviewed-on: https://chromium-review.googlesource.com/c/1355840
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614857}
[modify] https://crrev.com/13f12c8d2783426248a4d5f77681c0947bc46b48/third_party/blink/renderer/platform/heap/persistent.h
[modify] https://crrev.com/13f12c8d2783426248a4d5f77681c0947bc46b48/third_party/blink/renderer/platform/heap/persistent_node.h
Project Member

Comment 16 by bugdroid1@chromium.org, Dec 10 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd01dfdfc73cd2f927ad177c48a77f1c07c760aa

commit cd01dfdfc73cd2f927ad177c48a77f1c07c760aa
Author: Jeremy Roman <jbroman@chromium.org>
Date: Mon Dec 10 15:34:26 2018

Move Database::opened to std::atomic_bool.

Bug:  736037 
Change-Id: I645ec8a5871e8ed90b620fe28903ddb54fe4cc5b
Reviewed-on: https://chromium-review.googlesource.com/c/1368034
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615125}
[modify] https://crrev.com/cd01dfdfc73cd2f927ad177c48a77f1c07c760aa/third_party/blink/renderer/modules/webdatabase/database.cc
[modify] https://crrev.com/cd01dfdfc73cd2f927ad177c48a77f1c07c760aa/third_party/blink/renderer/modules/webdatabase/database.h
Project Member

Comment 17 by bugdroid1@chromium.org, Dec 10 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ab061a172c4452443e3471bc21d21d6a34412e18

commit ab061a172c4452443e3471bc21d21d6a34412e18
Author: Jeremy Roman <jbroman@chromium.org>
Date: Mon Dec 10 23:43:19 2018

Use base::AtomicFlag in CryptoResultCancel.

This replaces the use of WTF atomics. As a bonus, we gain a DCHECK-only
sequence checker for this use.

Since this only has one implementation (and there is no reason to want
an alternative implementation), it has also been devirtualized.

Bug:  736037 
Change-Id: I87b73e032b0de5bdc3e50a1ee849c3056355417e
Reviewed-on: https://chromium-review.googlesource.com/c/1369722
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615323}
[modify] https://crrev.com/ab061a172c4452443e3471bc21d21d6a34412e18/third_party/blink/renderer/DEPS
[modify] https://crrev.com/ab061a172c4452443e3471bc21d21d6a34412e18/third_party/blink/renderer/modules/crypto/crypto_result_impl.cc
[modify] https://crrev.com/ab061a172c4452443e3471bc21d21d6a34412e18/third_party/blink/renderer/modules/crypto/crypto_result_impl.h
[modify] https://crrev.com/ab061a172c4452443e3471bc21d21d6a34412e18/third_party/blink/renderer/platform/crypto_result.h
[modify] https://crrev.com/ab061a172c4452443e3471bc21d21d6a34412e18/third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
Project Member

Comment 18 by bugdroid1@chromium.org, Dec 11 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/97d45bc021d18994864d653fcec1145025a5a7d4

commit 97d45bc021d18994864d653fcec1145025a5a7d4
Author: Jeremy Roman <jbroman@chromium.org>
Date: Tue Dec 11 15:50:03 2018

Use std::atomic for DeferredTaskHandler::audio_thread_ and all other atomics in webaudio.

These are the ones that didn't explicitly include wtf/atomics.h and so were missed earlier.

Bug:  736037 ,247328
Change-Id: I407d1ad6f53c058866bb4a98ca910768fe9ce6c5
Reviewed-on: https://chromium-review.googlesource.com/c/1370187
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615536}
[modify] https://crrev.com/97d45bc021d18994864d653fcec1145025a5a7d4/third_party/blink/renderer/modules/webaudio/audio_buffer_source_node.h
[modify] https://crrev.com/97d45bc021d18994864d653fcec1145025a5a7d4/third_party/blink/renderer/modules/webaudio/audio_scheduled_source_node.h
[modify] https://crrev.com/97d45bc021d18994864d653fcec1145025a5a7d4/third_party/blink/renderer/modules/webaudio/deferred_task_handler.cc
[modify] https://crrev.com/97d45bc021d18994864d653fcec1145025a5a7d4/third_party/blink/renderer/modules/webaudio/deferred_task_handler.h
[modify] https://crrev.com/97d45bc021d18994864d653fcec1145025a5a7d4/third_party/blink/renderer/modules/webaudio/realtime_analyser.cc
[modify] https://crrev.com/97d45bc021d18994864d653fcec1145025a5a7d4/third_party/blink/renderer/modules/webaudio/realtime_analyser.h
Project Member

Comment 19 by bugdroid1@chromium.org, Dec 11 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/44502ac0122e17034e6837d566bf0e6f243e2b7d

commit 44502ac0122e17034e6837d566bf0e6f243e2b7d
Author: Jeremy Roman <jbroman@chromium.org>
Date: Tue Dec 11 16:22:40 2018

Update HeapTest to use modern atomics.

A combination of std::atomic (for a raw GC count) and appropriate higher-level
primitives from base is used to replace the current use of atomics.

Bug:  736037 
Change-Id: Id4856d4b8d21ff989694bbece70c61d86c686323
Reviewed-on: https://chromium-review.googlesource.com/c/1369720
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615552}
[modify] https://crrev.com/44502ac0122e17034e6837d566bf0e6f243e2b7d/third_party/blink/renderer/platform/heap/heap_test.cc

Comment 21 by jbroman@chromium.org, Dec 11

Status: Fixed (was: Started)
►

Sign in to add a comment