Closed Bug 1346592 Opened 3 years ago Closed 3 years ago

Can't stop profiling threads that were profiled at any point during their lifetime

Categories

(Core :: Gecko Profiler, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Steps to reproduce:
 1. Use the add-on from https://perf-html.io/ to profile your browser with the default settings.
 2. Notice that the Compositor thread shows up in profiles that you grab with these settings.
 3. Open the panel, remove the part ",Compositor" from the threads textbox (so that it's just "GeckoMain"), and click "Apply (Restart Profiler)".
 4. Grab another profile.

Expected results:
No more Compositor thread in the profile.

Actual results:
This new profile still contains the Compositor thread.
It looks like ThreadInfo only has a way to set mHasProfile to true (using ThreadInfo::SetHasProfile()), it's never set to false again. We should set it to false if the profiler is started and ShouldProfileThread returns false for the thread.
Comment on attachment 8858651 [details]
Bug 1346592 - When stopping the profiler, mark all ThreadInfos as not being profiled.

https://reviewboard.mozilla.org/r/130628/#review133536

::: tools/profiler/core/ThreadInfo.h:33
(Diff revision 1)
>  
>    mozilla::NotNull<PseudoStack*> Stack() const { return mPseudoStack; }
>  
> -  void SetHasProfile() { mHasProfile = true; }
> +  void StartSampling();
> +  void StopSampling();
> +  bool IsBeingProfiled() { return mIsBeingProfiled; }

The code already uses "profiling" in some places and "sampling" in others. Nonetheless, it's probably best to avoid mixing the terms for functions as closely related as these! Use {Start,Stop}Profiling()?

::: tools/profiler/core/platform.cpp:1865
(Diff revision 1)
>    NotNull<PseudoStack*> pseudoStack = info->Stack();
>  
>    tlsPseudoStack.set(pseudoStack.get());
>  
>    if (ShouldProfileThread(aLock, info)) {
> -    info->SetHasProfile();
> +    if (gPS->IsActive(aLock)) {

We might as well merge these two conditions, and put IsActive() first now.

::: tools/profiler/core/platform.cpp:2487
(Diff revision 1)
> -  if (gPS->FeatureJS(aLock)) {
> -    PS::ThreadVector& liveThreads = gPS->LiveThreads(aLock);
> +  PS::ThreadVector& liveThreads = gPS->LiveThreads(aLock);
> -    for (uint32_t i = 0; i < liveThreads.size(); i++) {
> +  for (uint32_t i = 0; i < liveThreads.size(); i++) {
> -      ThreadInfo* info = liveThreads.at(i);
> +    ThreadInfo* info = liveThreads.at(i);
> -      if (ShouldProfileThread(aLock, info)) {
> -        MOZ_RELEASE_ASSERT(info->HasProfile());
> +    if (info->IsBeingProfiled()) {
> +      if (gPS->FeatureJS(aLock)) {

I recently changed these lines to fix bug 1355807. I think the other parts of this patch mean that undoing my changes from that bug is ok here, but I want to double-check that you intended this and it's not an accidental reversion due to rebasing.
Attachment #8858651 - Flags: review?(n.nethercote) → review+
Comment on attachment 8858651 [details]
Bug 1346592 - When stopping the profiler, mark all ThreadInfos as not being profiled.

https://reviewboard.mozilla.org/r/130628/#review133536

> I recently changed these lines to fix bug 1355807. I think the other parts of this patch mean that undoing my changes from that bug is ok here, but I want to double-check that you intended this and it's not an accidental reversion due to rebasing.

Oh, interesting. I hadn't fully understood the patch from bug 1355807 when I looked at it last week, but now that I've written this patch, it makes a lot more sense. And yes, I believe the other changes from this patch should make the revert ok.
In particular, the call to StopProfiling() right after this line will make sure that we won't enter this branch again for this thread unless StartProfiling has been called in the meantime, in which case we will also have called startJSSampling, which allows us to call stopJSSampling again.
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/1b277ae21ba5
When stopping the profiler, mark all ThreadInfos as not being profiled. r=njn
I caught that mistake before I landed, refreshed the patch to fix it, but then didn't push to mozreview before I clicked the autoland button. :(
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Flags: needinfo?(mstange)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/802a9bc40bc6
Backed out changeset 1b277ae21ba5
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/855059cc1406
When stopping the profiler, mark all ThreadInfos as not being profiled. r=njn
https://hg.mozilla.org/mozilla-central/rev/855059cc1406
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.