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

RESOLVED FIXED in Firefox 55

Status

()

Core
Gecko Profiler
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 months ago
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.
(Assignee)

Comment 1

a month ago
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 hidden (mozreview-request)

Comment 3

a month ago
mozreview-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

::: 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+
(Assignee)

Comment 4

a month ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)

Comment 6

a month ago
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

Comment 7

a month ago
backed out for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=92208573&repo=autoland&lineNumber=5024
Flags: needinfo?(mstange)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

a month ago
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)

Comment 10

a month ago
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/802a9bc40bc6
Backed out changeset 1b277ae21ba5

Comment 11

a month ago
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

Comment 12

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/855059cc1406
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.