I have a bunch of ideas for improving the handling of ThreadInfo.

Now that ThreadResponsiveness is only used on the main thread, we can refactor ThreadInfo a bit. This patch does the following. - Removes ThreadInfo::mThread, which is unused. - Changes ThreadInfo::mRespInfo to a Maybe<>, and moves the is-main-thread checking outside of ThreadInfo and ThreadResponsiveness. - Renames {ThreadInfo,TickSample}::mRespInfo as mResponsiveness, to better match the class name.

MaybeSetProfile() does a check and then sets on success. By separating the check from the set, we can avoid some subsequent HasProfile() checks.

Currently, when the profiler is active we hold onto the ThreadInfo of all threads that die. Then when capturing a profile we ignore all threads that aren't being profiled. This patch changes things so we only hold onto the ThreadInfos of threads that die if they are being profiled. In effect it removes state 3 from the following list of possible ThreadInfo states: 1. !PendingDelete + !HasProfile 2. !PendingDelete + HasProfile 3. PendingDelete + !HasProfile (no longer used) 4. PendingDelete + HasProfile

Currently, ThreadInfos for live and dead threads are stored in a single vector. This patch separates them into two separate vectors. This ensures that the two kinds of ThreadInfos can't be mixed up. It also means ThreadInfo::mPendingDelete can be removed.

Comment on attachment 8854709 [details] [diff] [review] (part 2) - Replace MaybeSetProfile() with ShouldProfileThread() Review of attachment 8854709 [details] [diff] [review]: ----------------------------------------------------------------- r+ w/ sanity check of the IsPendingDelete query. ::: tools/profiler/core/platform.cpp @@ +2355,5 @@ > > + if (ShouldProfileThread(aLock, info)) { > + info->SetHasProfile(); > + > + if (info->IsPendingDelete()) { Is this correct? It looks as if the reinitializeOnResume call used to be guarded by !info->IsPendingDelete() but is now guarded by info->IsPendingDelete().

Comment on attachment 8854723 [details] [diff] [review] (part 3) - Don't hold onto ThreadInfos for dead threads that aren't being profiled Review of attachment 8854723 [details] [diff] [review]: ----------------------------------------------------------------- Consider adding some part of the changeset comment in the code? It's a helpful comment.

Comment on attachment 8854724 [details] [diff] [review] (part 4) - Separate ThreadInfos for live and dead threads Review of attachment 8854724 [details] [diff] [review]: ----------------------------------------------------------------- Looks OK. Nice tidyup. ::: tools/profiler/core/platform.cpp @@ +269,4 @@ > ProfileBuffer* mBuffer; > > + // Info on all the registered threads, both live and dead. ThreadIds in > + // mLiveThreads are unique. ThreadIds in mDeadThreads may not be, because Marginally OT, because I see you're not changing the semantics of this, but .. is there any consequence to the fact that ThreadIds in mDeadThreads are not guaranteed to be unique? Could that ever be a problem?

> > + if (info->IsPendingDelete()) { > > Is this correct? It looks as if the reinitializeOnResume > call used to be guarded by !info->IsPendingDelete() but is now > guarded by info->IsPendingDelete(). Ugh, you're right. I inverted the sense and changed the loop to |continue|, and then changed my mind but failed to re-invert. Thanks for catching.

> Ugh, you're right. I inverted the sense and changed the loop to |continue|, > and then changed my mind but failed to re-invert. Thanks for catching. The good news is that the gtest I added in bug 1351136 failed as a result of this. I'm also contemplating splitting ThreadInfo into separate LiveThreadInfo and DeadThreadInfo classes, which would make this kind of mistake harder or impossible.

> Marginally OT, because I see you're not changing the semantics > of this, but .. is there any consequence to the fact that > ThreadIds in mDeadThreads are not guaranteed to be unique? > Could that ever be a problem? Good question. I'm not sure. I suspect any problems in that case would manifest in display of profiles, e.g. the UI might conflate distinct threads. It might also depend on whether the threads have the same name. I've also wondered about a similar case with live threads. If a thread dies its tid is recycled, the data in the ProfileBuffer will be ambiguous. mstange, do you have any thoughts about this?

> The good news is that the gtest I added in bug 1351136 failed as a result of > this. I'm also contemplating splitting ThreadInfo into separate > LiveThreadInfo and DeadThreadInfo classes, which would make this kind of > mistake harder or impossible. Furthermore, part 4 removed that inverted condition. That likely explains why I didn't catch it before uploading.

> Consider adding some part of the changeset comment in the code? It's a > helpful comment. Part 4 contains this comment, which I thinks suffices: > // Info on all the registered threads, both live and dead. ThreadIds in > // mLiveThreads are unique. ThreadIds in mDeadThreads may not be, because > // ThreadIds can be reused. HasProfile() is true for all ThreadInfos in > // mDeadThreads because we don't hold onto ThreadInfos for non-profiled dead > // threads. > ThreadVector mLiveThreads; > ThreadVector mDeadThreads;

(In reply to Nicholas Nethercote [:njn] from comment #11) > > Marginally OT, because I see you're not changing the semantics > > of this, but .. is there any consequence to the fact that > > ThreadIds in mDeadThreads are not guaranteed to be unique? > > Could that ever be a problem? > > Good question. I'm not sure. I suspect any problems in that case would > manifest in display of profiles, e.g. the UI might conflate distinct > threads. It might also depend on whether the threads have the same name. > > I've also wondered about a similar case with live threads. If a thread dies > its tid is recycled, the data in the ProfileBuffer will be ambiguous. > > mstange, do you have any thoughts about this? The only time that perf.html looks at the thread's id is when it specifies the tooltip for the thread name in the thread list. We don't use it as a key in a map, or anything like that. Having multiple threads with the same tid is not a problem for the frontend. The profile buffer point that you bring up is valid, as far as I can tell. In that case we'd probably assign the samples of either thread to both threads. That will be slightly misleading but it seems rare enough that I wouldn't worry about it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bf903b1e7ccdd88a688b988756e78ddd7014741 Bug 1353630 (part 1) - Refactor ThreadResponsiveness use in ThreadInfo. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/36e5ca3911b82d759dbc39cd5a5aac10a340585e Bug 1353630 (part 2) - Replace MaybeSetProfile() with ShouldProfileThread(). r=jseward. https://hg.mozilla.org/integration/mozilla-inbound/rev/c43ee7462e5585c8c3b1c78f0a650580feabc0c8 Bug 1353630 (part 3) - Don't hold onto ThreadInfos for dead threads that aren't being profiled. r=jseward. https://hg.mozilla.org/integration/mozilla-inbound/rev/e02bb02196aaeb03763ceada5697ad883d8c75af Bug 1353630 (part 4) - Separate ThreadInfos for live and dead threads. r=jseward. https://hg.mozilla.org/integration/mozilla-inbound/rev/fddfbadf8fed92917b43e88020be9feff8cb2537 Bug 1353630 (part 5) - Allocate PseudoStack within ThreadInfo's constructor. r=jseward.

https://hg.mozilla.org/mozilla-central/rev/6bf903b1e7cc https://hg.mozilla.org/mozilla-central/rev/36e5ca3911b8 https://hg.mozilla.org/mozilla-central/rev/c43ee7462e55 https://hg.mozilla.org/mozilla-central/rev/e02bb02196aa https://hg.mozilla.org/mozilla-central/rev/fddfbadf8fed

Comment on attachment 8854724 [details] [diff] [review] (part 4) - Separate ThreadInfos for live and dead threads Review of attachment 8854724 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/profiler/core/platform.cpp @@ +1169,5 @@ > +#ifdef MOZ_TASK_TRACER > +static void > +StreamNameAndThreadId(const char* aName, int aThreadId) > +{ > + aWriter.StartObjectElement(); This (--enable-tasktracer -only) code does not compile because there is no aWriter argument.

> This (--enable-tasktracer -only) code does not compile because there is no > aWriter argument. Apologies for the bustage. I'll fix this up next week.

I've put up a patch for it in bug 1356752.