Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1956333002:
Change the base class of BlameContext into AsyncEnabledStateObserver (Closed)
|
Created:
4 years, 7 months ago by Xiaocheng Modified:
4 years, 7 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@ThreadSafeObserver Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange the base class of BlameContext into AsyncEnabledStateObserver
This is a follow-up CL of https://codereview.chromium.org/1956323002.
BUG=610629
Committed: https://crrev.com/31ea0222e9a5124bd55a38ce9bec31a9623bb393
Cr-Commit-Position: refs/heads/master@{#393297}
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : Fix component unit test TaskQueueManagerTestWithTracing.BlameContextAttribution #Patch Set 4 : Rebased #Patch Set 5 : Rebased #Patch Set 6 : Use ThreadChecker #Patch Set 7 : Add missing #include #Patch Set 8 : Remove thread checking in Enter() and Leave() #Patch Set 9 : Add note on thread safety for Enter() and Leave() #
Messages
Total messages: 34 (13 generated)
Description was changed from ========== Change the base class of BlameContext into ThreadSafeEnabledStateObserver This is a follow-up CL of https://codereview.chromium.org/1956323002. BUG=to be added ========== to ========== Change the base class of BlameContext into AsyncEnabledStateObserver This is a follow-up CL of https://codereview.chromium.org/1956323002. BUG=610629 ==========
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956333002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1956333002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956333002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1956333002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956333002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1956333002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
xiaochengh@chromium.org changed reviewers: + primiano@chromium.org, skyostil@chromium.org
PTAL. This is a follow-up CL of https://codereview.chromium.org/1956323002 making BlameContext thread safe.
lgtm, thanks!
On 2016/05/11 11:48:50, Sami wrote: > lgtm, thanks! Oh no this is now breaking the entire tracing threading model
On 2016/05/11 12:09:40, Primiano Tucci wrote: > On 2016/05/11 11:48:50, Sami wrote: > > lgtm, thanks! > > Oh no this is now breaking the entire tracing threading model I am just kidding. LGTM. :D P.S: Maybe you might add a threadchecker in BlameContext to ensure ctor, dtor and snapshot are always called on the same thread .
On 2016/05/11 12:09:40, Primiano Tucci wrote: > On 2016/05/11 11:48:50, Sami wrote: > > lgtm, thanks! > > Oh no this is now breaking the entire tracing threading model Okay, that was scary :D
Thanks for the scary and non-scary comments.
I added ThreadChecker into the class, which turns out to be quite helpful --- it
breaks a browser test (see try job results in Patch 7). The reason is that there
are call sites of BlameContext::Enter() and Leave() in V8GCController.cpp, which
can be run from a non-main thread.
Good news is that this trouble-making blame context being accessed by multiple
threads is TopLevelBlameContext. Since it is never modified after initialization
and lives indefinitely with blink::Platform, it is fine to allow access from all
threads concurrently.
So I just removed thread checking from Enter() and Leave(), and kept only those
in dtor, Initialize() and TakeSnapshot(). Multi-thread access to
TopLevelBlameContext is allowed as a special case. For all other blame contexts,
we should still make sure that all methods (including Enter and Leave) are
called from the same thread.
Maybe we should introduce a new member function AllowMultiThreadAccess() like
follows:
class BlameContext {
virtual bool AllowMultiThreadAccess() const { return false; }
...
};
class TopLevelBlameContext : public BlameContext {
bool AllowMultiThreadAccess() const override { return true; }
};
void BlameContext::~BlameContext/Enter/Leave/TakeSnapshot/Initialize() {
DCHECK(thread_checker_.CalledOnValidThread() || AllowMultiThreadAccess());
...
}
On 2016/05/12 at 05:55:44, Xiaocheng wrote:
> Thanks for the scary and non-scary comments.
>
> I added ThreadChecker into the class, which turns out to be quite helpful ---
it breaks a browser test (see try job results in Patch 7). The reason is that
there are call sites of BlameContext::Enter() and Leave() in V8GCController.cpp,
which can be run from a non-main thread.
>
> Good news is that this trouble-making blame context being accessed by multiple
threads is TopLevelBlameContext. Since it is never modified after initialization
and lives indefinitely with blink::Platform, it is fine to allow access from all
threads concurrently.
>
> So I just removed thread checking from Enter() and Leave(), and kept only
those in dtor, Initialize() and TakeSnapshot(). Multi-thread access to
TopLevelBlameContext is allowed as a special case. For all other blame contexts,
we should still make sure that all methods (including Enter and Leave) are
called from the same thread.
>
> Maybe we should introduce a new member function AllowMultiThreadAccess() like
follows:
>
> class BlameContext {
> virtual bool AllowMultiThreadAccess() const { return false; }
> ...
> };
>
> class TopLevelBlameContext : public BlameContext {
> bool AllowMultiThreadAccess() const override { return true; }
> };
>
> void BlameContext::~BlameContext/Enter/Leave/TakeSnapshot/Initialize() {
> DCHECK(thread_checker_.CalledOnValidThread() || AllowMultiThreadAccess());
> ...
> }
Some further thought: it seems that thread affinity checking is a little bit
more complicated than we thought due to multi-thread access to
TopLevelBlameContext. I'm thinking about just landing Patch 5 as is, and upload
another patch for thread checking.
WDYT?
On 2016/05/12 08:21:13, Xiaocheng wrote: > Some further thought: it seems that thread affinity checking is a little bit > more complicated than we thought due to multi-thread access to > TopLevelBlameContext. I'm thinking about just landing Patch 5 as is, and upload > another patch for thread checking. > > WDYT? Up to skyostil@ to decide. SGTM from my viewpoint as that (lack of checks) is already the case today. Would be nice though if these things got sorted out, as they can negatively impact go/slow-reports (where tracing is enabled in background) and turn all those into crashes.
Right, sorry I forgot about the V8GCController case. I think we do actually want to support Entering() and Leaving() from different threads because of that use case. Like you said, it is safe because a) Enter/Leave access fields which aren't mutated and b) the user is responsible for guaranteeing lifetimes. For in V8GCController's case the worker threads are torn down before the main thread is. I think landing this as is lgtm. If you want you could add a note to Enter/Leave saying that they can be called from any thread as long as object lifetime is controlled.
The CQ bit was checked by xiaochengh@chromium.org
Added a note on thread safety to Enter() and Leave(), following Sami's suggestions. I'm going directly to commit (and go to sleep) since there is no more comment to address.
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1956333002/#ps160001 (title: "Add note on thread safety for Enter() and Leave()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956333002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1956333002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by skyostil@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956333002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1956333002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Change the base class of BlameContext into AsyncEnabledStateObserver This is a follow-up CL of https://codereview.chromium.org/1956323002. BUG=610629 ========== to ========== Change the base class of BlameContext into AsyncEnabledStateObserver This is a follow-up CL of https://codereview.chromium.org/1956323002. BUG=610629 Committed: https://crrev.com/31ea0222e9a5124bd55a38ce9bec31a9623bb393 Cr-Commit-Position: refs/heads/master@{#393297} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/31ea0222e9a5124bd55a38ce9bec31a9623bb393 Cr-Commit-Position: refs/heads/master@{#393297} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
