Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2565893002:
Fix TSAN race in access to StatisticsRecorder::lock_ (Closed)
|
Created:
4 years ago by gab Modified:
4 years ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org, fdoray Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix TSAN race in access to StatisticsRecorder::lock_
BUG=672852
TEST=No more content_unittests, events_unittests, and extensions_unittests failures on linux_chromium_tsan_rel_ng
Confirmed @ https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_tsan_rel_ng/builds/377
Committed: https://crrev.com/c435080b8ebaca0d833b2c2e261eafa2090f725d
Cr-Commit-Position: refs/heads/master@{#437932}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : nit #
Messages
Total messages: 26 (18 generated)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
gab@chromium.org changed reviewers: + asvitkine@chromium.org
Alexei PTAL. Basically the "culprit" CL which triggered a TSAN failure is merely adding a SCOPED_UMA_HISTOGRAM_TIMER in a path that is sometimes also hit prior to StatisticsRecorder::Initialize(). It's okay that these histogram hits be dropped but the problem is that TSAN trips on a racy access to StatisticsRecorder::lock_ (it's racily accessed in static StatisticsRecorder::FindHistogram() and the Lock's creation in StatisticsRecorder::StatisticsRecorder()). Making the lock into a LazyInstance solves this issue. This should have virtually no impact on performance since the lock_ is pretty much always instantiated in practice except *very* early in the process.
Description was changed from ========== Fix TSAN race in access to StatisticsRecorder::lock_ BUG=672852 ========== to ========== Fix TSAN race in access to StatisticsRecorder::lock_ BUG=672852 TEST=No more content_unittests, events_unittests, and extensions_unittests failures on linux_chromium_tsan_rel_ng ==========
On 2016/12/09 22:50:56, gab wrote: > Alexei PTAL. > > Basically the "culprit" CL which triggered a TSAN failure is merely adding a > SCOPED_UMA_HISTOGRAM_TIMER in a path that is sometimes also hit prior to > StatisticsRecorder::Initialize(). > > It's okay that these histogram hits be dropped but the problem is that TSAN > trips on a racy access to StatisticsRecorder::lock_ (it's racily accessed in > static StatisticsRecorder::FindHistogram() and the Lock's creation in > StatisticsRecorder::StatisticsRecorder()). > > Making the lock into a LazyInstance solves this issue. > > This should have virtually no impact on performance since the lock_ is pretty > much always instantiated in practice except *very* early in the process. CC fdoray FYI
Generally looks fine, but I'm concerned about SCOPED_UMA_HISTOGRAM_TIMER() before StatisticsRecorder::Initialize. I thought we call StatisticsRecorder::Initialize() very early in single threaded mode. Is there just non-determinism about which happens first? Can StatisticsRecorder::Initialize() be moved earlier? https://codereview.chromium.org/2565893002/diff/20001/base/metrics/statistics... File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/2565893002/diff/20001/base/metrics/statistics... base/metrics/statistics_recorder.cc:112: return !!histograms_; Nit: I find != nullptr more readable.
On 2016/12/09 23:20:11, Alexei Svitkine (slow) wrote: > Generally looks fine, but I'm concerned about SCOPED_UMA_HISTOGRAM_TIMER() > before StatisticsRecorder::Initialize. > > I thought we call StatisticsRecorder::Initialize() very early in single threaded > mode. Is there just non-determinism about which happens first? > > Can StatisticsRecorder::Initialize() be moved earlier? See http://crbug.com/672852#c2 for stacks. Basically ThreadData::GetRetiredOrCreateThreadData gets called when every thread is created and Francois added a SCOPED_UMA_HISTOGRAM_TIMER to it to make sure that as the map grows the timings don't become out of hand. The racy call is in a test that creates a helper IOThread before even invoking the test harness (and thus that thread races with the test harness' StatisticsRecorder::Initialize). This won't be a problem in the product (nor is it even bad to miss recording the first timing here, we're really looking to make sure this doesn't become slow in long-standing sessions). https://cs.chromium.org/chromium/src/base/tracked_objects.cc?type=cs&q=SCOPED... https://codereview.chromium.org/2565893002/diff/20001/base/metrics/statistics... File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/2565893002/diff/20001/base/metrics/statistics... base/metrics/statistics_recorder.cc:112: return !!histograms_; On 2016/12/09 23:20:10, Alexei Svitkine (slow) wrote: > Nit: I find != nullptr more readable. Done.
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix TSAN race in access to StatisticsRecorder::lock_ BUG=672852 TEST=No more content_unittests, events_unittests, and extensions_unittests failures on linux_chromium_tsan_rel_ng ========== to ========== Fix TSAN race in access to StatisticsRecorder::lock_ BUG=672852 TEST=No more content_unittests, events_unittests, and extensions_unittests failures on linux_chromium_tsan_rel_ng Confirmed @ https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
lgtm
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481572813628960,
"parent_rev": "2afb4c1a09f43ff1a8e5ccc5bf37dbec44d02847", "commit_rev":
"fbcb714d1a0be8d8d386f298409132a3e5c31e32"}
Message was sent while issue was closed.
Description was changed from ========== Fix TSAN race in access to StatisticsRecorder::lock_ BUG=672852 TEST=No more content_unittests, events_unittests, and extensions_unittests failures on linux_chromium_tsan_rel_ng Confirmed @ https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... ========== to ========== Fix TSAN race in access to StatisticsRecorder::lock_ BUG=672852 TEST=No more content_unittests, events_unittests, and extensions_unittests failures on linux_chromium_tsan_rel_ng Confirmed @ https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... Review-Url: https://codereview.chromium.org/2565893002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix TSAN race in access to StatisticsRecorder::lock_ BUG=672852 TEST=No more content_unittests, events_unittests, and extensions_unittests failures on linux_chromium_tsan_rel_ng Confirmed @ https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... Review-Url: https://codereview.chromium.org/2565893002 ========== to ========== Fix TSAN race in access to StatisticsRecorder::lock_ BUG=672852 TEST=No more content_unittests, events_unittests, and extensions_unittests failures on linux_chromium_tsan_rel_ng Confirmed @ https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... Committed: https://crrev.com/c435080b8ebaca0d833b2c2e261eafa2090f725d Cr-Commit-Position: refs/heads/master@{#437932} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c435080b8ebaca0d833b2c2e261eafa2090f725d Cr-Commit-Position: refs/heads/master@{#437932} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
