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

Issue 1186053006: [tracing] fix a data race in MemoryDumpManager (Closed)

Created:
5 years, 6 months ago by Primiano Tucci (use gerrit)
Modified:
5 years, 6 months ago
Reviewers:
petrcermak
CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, erikwright+watch_chromium.org, skobes
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[tracing] fix a data race in MemoryDumpManager Fix a data race spotted by the TSan bot [1] and re-enable the MemoryDumpManagerTest.RespectTaskRunnerAffinity test. The race consisted in mistakenly looking into the pmd_holder->num_pending_async_requests to determine whether the dump should be finalized inline at the end of the current stack frame or on the last thread hop. In theory it is possible that the chain of posted threads is done before hitting the num_pending_async_requests check. [1] http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20TSan%20Tests/builds/5757 BUG=499983 Committed: https://crrev.com/5dc4c114e510b6d6a9d0c5760a8fba17186d0966 Cr-Commit-Position: refs/heads/master@{#334396}

Patch Set 1 #

Total comments: 1

Patch Set 2 : femto perf improvement #

Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -10 lines) Patch
M base/trace_event/memory_dump_manager.cc View 1 3 chunks +5 lines, -2 lines 0 comments Download
M base/trace_event/memory_dump_manager_unittest.cc View 1 chunk +1 line, -8 lines 0 comments Download

Messages

Total messages: 9 (3 generated)
Primiano Tucci (use gerrit)
5 years, 6 months ago (2015-06-15 12:07:31 UTC) #2
Primiano Tucci (use gerrit)
5 years, 6 months ago (2015-06-15 12:35:09 UTC) #3
petrcermak
LGTM. Thanks, Petr https://codereview.chromium.org/1186053006/diff/1/base/trace_event/memory_dump_manager.cc File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1186053006/diff/1/base/trace_event/memory_dump_manager.cc#newcode320 base/trace_event/memory_dump_manager.cc:320: did_post_any_async_task |= did_post_async_task; As Simon suggested, ...
5 years, 6 months ago (2015-06-15 14:00:26 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1186053006/20001
5 years, 6 months ago (2015-06-15 14:04:07 UTC) #7
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years, 6 months ago (2015-06-15 17:06:49 UTC) #8
commit-bot: I haz the power
5 years, 6 months ago (2015-06-15 17:07:37 UTC) #9
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/5dc4c114e510b6d6a9d0c5760a8fba17186d0966
Cr-Commit-Position: refs/heads/master@{#334396}

Powered by Google App Engine
This is Rietveld 408576698