Closed
Bug 1351963
Opened 7 years ago
Closed 7 years ago
Get rid of fake ThreadInfos in profiler_get_backtrace()
Categories
(Core :: Gecko Profiler, enhancement)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(9 files)
2.78 KB,
patch
|
mstange
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
8.80 KB,
patch
|
mstange
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
4.61 KB,
patch
|
mstange
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
10.11 KB,
patch
|
jseward
:
review+
|
Details | Diff | Splinter Review |
7.86 KB,
patch
|
jseward
:
review+
|
Details | Diff | Splinter Review |
13.35 KB,
patch
|
jseward
:
review+
|
Details | Diff | Splinter Review |
6.53 KB,
patch
|
jseward
:
review+
|
Details | Diff | Splinter Review |
10.50 KB,
patch
|
jseward
:
review+
|
Details | Diff | Splinter Review |
2.98 KB,
patch
|
jseward
:
review+
|
Details | Diff | Splinter Review |
The profiler takes two kinds of samples. This first kind is periodic samples (via SamplerThread::Run()). These are the "normal" ones and they are fairly straightforward. The second kind is synchronous samples (via profiler_get_backtrace()). These ones are weirder. In particular, we create a fresh, fake ThreadInfo that gets stored in ProfilerBacktrace. Furthermore, this fake ThreadInfo has a pointer to the real PseudoStack, and it's possible that the ProfilerBacktrace might outlive the thread and thus the PseudoStack, resulting in a dangling pointer. It turns out this doesn't matter because the pointer is never used, but this is by luck rather than good management. I want to fix this up. - No fake ThreadInfos in profiler_get_backtrace(), because they are the root of the problem. - This means that TickSample can't require a ThreadInfo. That's doable, because Tick() only needs a few things that the ThreadInfo (real or fake) provides, and we can put them directly into TickSample. - This also means that ProfilerBacktrace can't store a ThreadInfo. That's also doable. - Once all that's done, there's a simple 1-to-1 relationship between (real) ThreadInfos and PseudoStacks, and so we can simplify things so that each ThreadInfo unconditionally owns a PseudoStack.
Assignee | ||
Comment 1•7 years ago
|
||
It doesn't need any virtual methods, nor the FRIEND_TEST declarations.
Attachment #8853236 -
Flags: review?(mstange)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
StreamSamplesAndMarkers() is the only ThreadInfo method called on ProfilerBacktrace::mThreadInfo. Furthermore, it doesn't use all that much stuff from ThreadInfo, and what stuff it does use we can instead pass in as arguments. This patch moves StreamSamplesAndMarkers() out of the class. It's a little ugly, but a necessary precursor for removing ProfilerBacktrace::mThreadInfo and all the subsequent improvements.
Attachment #8853237 -
Flags: review?(mstange)
Assignee | ||
Comment 3•7 years ago
|
||
At this point the only things in the ThreadInfo it uses are the thread name and id, which are easy to store instead. This gets a step closer to avoiding the use of ThreadInfo in profiler_get_backtrace().
Attachment #8853238 -
Flags: review?(mstange)
Assignee | ||
Comment 4•7 years ago
|
||
LastSample only makes sense for periodic samples, which are written to the global ProfileBuffer. It doesn't make sense for synchronous samples which are written to their own unshared buffer. At the moment it doesn't hurt to use them in this nonsensical way, but the ThreadInfo profiler_get_backtrace() will be removed soon, and we won't even have a LastSample to use nonsensically. So this patch makes the LastSample argument to addTagThreadId() optional. Which means we have to pass in a ThreadId, so there's no longer much point duplicating the ThreadId in LastSample, so the patch removes that field too. This avoids the possibility of the duplicate ThreadId failing to match, which is nice.
Attachment #8853239 -
Flags: review?(jseward)
Assignee | ||
Comment 5•7 years ago
|
||
This patch does the following. - Splits TickSample's constructor in two, one for the periodic sample case, and one for the synchronous sample case, and initializes more stuff in them. (The two constructors aren't that different right now, but they will become more different when I remove TickSample::mThreadInfo.) - Makes all the constructor-filled fields in TickSample |const|. - Reorders the fields so that the constructor-filled ones are before the ones that get filled in later. - Omits mContext on Mac via conditional compilation, to make the omission clearer.
Attachment #8853240 -
Flags: review?(jseward)
Assignee | ||
Comment 6•7 years ago
|
||
This avoids the need for the fake ThreadInfo in profiler_get_backtrace(). It requires adding a few extra fields to TickSample.
Attachment #8853241 -
Flags: review?(jseward)
Assignee | ||
Comment 7•7 years ago
|
||
The patch also adds a MOZ_RELEASE_ASSERT in profiler_unregister_thread() for the case where the ThreadInfo isn't found, which is informative.
Attachment #8853242 -
Flags: review?(jseward)
Assignee | ||
Comment 8•7 years ago
|
||
Currently each live thread has a PseudoStack that is owned by tlsPseudoStack, and a ThreadInfo that has a non-owning pointer to the same PseudoStack. Then, if the profile is active when the thread dies, ownership of the PseudoStack is transferred to the ThreadInfo. This patch simplifies the ownership rules. Every ThreadInfo now always owns its PseudoStack and is responsible for destroying it. tlsPseudoStack is a non-owning pointer, and so must be cleared when a PseudoStack is destroyed. This simplifies the code in a few places.
Attachment #8853243 -
Flags: review?(jseward)
Assignee | ||
Comment 9•7 years ago
|
||
SetSampleContext() sets the TickSample's register fields, and the two callers of SetSampleContext() set the TickSample's mContext. This patch changes SetSampleContext() so it sets all the fields in one place. It also renames SetSampleContext() as FillInSample(), because it sets more than just the context.
Attachment #8853244 -
Flags: review?(jseward)
Updated•7 years ago
|
Attachment #8853236 -
Flags: review?(mstange) → review+
Comment 10•7 years ago
|
||
Comment on attachment 8853237 [details] [diff] [review] (part 2) - Move StreamSamplesAndMarkers() out of ThreadInfo Review of attachment 8853237 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/profiler/core/ProfilerBacktrace.cpp @@ +30,5 @@ > const TimeStamp& aStartTime, > UniqueStacks& aUniqueStacks) > { > + // We can use nullptr for aPseudoContext here because the only PseudoStack > + // field that ProfilerBacktrace uses is mContext, and only for This comment is confusing because the argument is called aContext and not aPseudoContext, and the part about "the only PseudoStack field" seems irrelevant because we don't pass the PseudoStack to this method. I might be misunderstanding.
Attachment #8853237 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8853238 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 11•7 years ago
|
||
> > + // We can use nullptr for aPseudoContext here because the only PseudoStack
> > + // field that ProfilerBacktrace uses is mContext, and only for
>
> This comment is confusing because the argument is called aContext and not
> aPseudoContext, and the part about "the only PseudoStack field" seems
> irrelevant because we don't pass the PseudoStack to this method. I might be
> misunderstanding.
Yes, the comment in front of the nullptr arg should read |aPseudoContext| instead of |aContext|.
The point is that a PseudoStack argument is only necessary for some calls to StreamSamplesAndMarkers(), and this is *not* one of those calls. I will rewrite the comment to make this clearer.
Assignee | ||
Comment 12•7 years ago
|
||
Oh! The argument is actually a JSContext, not a PseudoStack. I will reword it again :)
Assignee | ||
Comment 13•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5af677c09ae7ab833e439c4b1ddd4cbdc35583a8 Bug 1351963 (part 1) - Tidy up ThreadInfo. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ce5cdb4e5a59daac066a2bd59abbf48388795e Bug 1351963 (part 2) - Move StreamSamplesAndMarkers() out of ThreadInfo. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/016c13131fffb242bdb06a33b4946a96c175a603 Bug 1351963 (part 3) - Remove ThreadInfo from ProfilerBacktrace. r=mstange.
Assignee | ||
Updated•7 years ago
|
Attachment #8853236 -
Flags: checkin+
Assignee | ||
Updated•7 years ago
|
Attachment #8853237 -
Flags: checkin+
Assignee | ||
Updated•7 years ago
|
Attachment #8853238 -
Flags: checkin+
Comment 14•7 years ago
|
||
backed out part 2 & 3 in case this is causing leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=88226147&repo=mozilla-inbound
Flags: needinfo?(n.nethercote)
Comment 15•7 years ago
|
||
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/abd1a8acd193 Backed out changeset 016c13131fff https://hg.mozilla.org/integration/mozilla-inbound/rev/661462056254 Backed out changeset d9ce5cdb4e5a for hopefully fix the dt leaks
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5af677c09ae7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 17•7 years ago
|
||
Comment on attachment 8853239 [details] [diff] [review] (part 4) - Make the LastSample argument to addTagThreadId optional Review of attachment 8853239 [details] [diff] [review]: ----------------------------------------------------------------- r+ with LastSample::LastSample fixed up. ::: tools/profiler/core/ProfileBuffer.h @@ +25,2 @@ > struct LastSample { > explicit LastSample(int aThreadId) You now don't need to pass in |aThreadId|.
Attachment #8853239 -
Flags: review?(jseward) → review+
Comment 18•7 years ago
|
||
Comment on attachment 8853240 [details] [diff] [review] (part 5) - Improve TickSample Review of attachment 8853240 [details] [diff] [review]: ----------------------------------------------------------------- You're sure you want to use the terminology synchronous-vs-periodic as opposed to either sync-vs-async (a traditional pairing) or something like periodic-vs-ondemand? ::: tools/profiler/core/platform.cpp @@ +394,4 @@ > void PopulateContext(tick_context_t* aContext); > > + // False for periodic samples, true for synchronous samples. > + const bool mIsSamplingCurrentThread; I don't entirely like the name mIsSamplingCurrentThread ("Current" is vague) but that said I can't think of anything that's much better. mIsSamplingThisThread? mIsSamplingCallingThread? no. Given the use of "synchronous" vs "periodic", perhaps mIsSynchronousSample ? @@ +406,5 @@ > + // The remaining fields are filled in, after construction, by > + // SamplerThread::SuspendAndSampleAndResume() for periodic samples, and > + // PopulateContext() for synchronous samples. They are filled in separately > + // from the other fields in this class because the code that fills them in is > + // platform-specific. platform-specific. platform-specific appears twice.
Attachment #8853240 -
Flags: review?(jseward) → review+
Updated•7 years ago
|
Attachment #8853241 -
Flags: review?(jseward) → review+
Comment 19•7 years ago
|
||
Comment on attachment 8853242 [details] [diff] [review] (part 7) - Factor out repeated thread-finding code Review of attachment 8853242 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/profiler/core/platform.cpp @@ +1872,5 @@ > } > NotNull<PseudoStack*> stack = WrapNotNull(new PseudoStack()); > tlsPseudoStack.set(stack); > > + ThreadInfo* info = new ThreadInfo(aName, Thread::GetCurrentId(), The effect of this is to replace one call to Thread::GetCurrentId() with two, right? Does that matter?
Attachment #8853242 -
Flags: review?(jseward) → review+
Updated•7 years ago
|
Attachment #8853243 -
Flags: review?(jseward) → review+
Comment 20•7 years ago
|
||
Comment on attachment 8853244 [details] [diff] [review] (part 9) - Improve SetSampleContext() Review of attachment 8853244 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/profiler/core/platform-linux-android.cpp @@ +76,5 @@ > return gettid(); > } > > static void > +FillInSample(TickSample* sample, ucontext_t* aContext) Could aContext be a ucontext_t& or even a const ucontext_t&? IIUC we're not modifying it.
Attachment #8853244 -
Flags: review?(jseward) → review+
Assignee | ||
Comment 21•7 years ago
|
||
> You're sure you want to use the terminology synchronous-vs-periodic > as opposed to either sync-vs-async (a traditional pairing) or something > like periodic-vs-ondemand? Good question. "Synchronous" was already in use, and I invented "periodic". I think I'll stick with that; it's fairly descriptive and I don't like "asynchronous" at all for the periodic samples, because that to me suggests that we make a request and some non-deterministic time later it completes. I will augment the comments a little to make things clearer. > I don't entirely like the name mIsSamplingCurrentThread ("Current" is vague) > but that said I can't think of anything that's much better. > mIsSamplingThisThread? mIsSamplingCallingThread? no. Given the use of > "synchronous" vs "periodic", perhaps mIsSynchronousSample ? I thought about this and decided to leave it alone. But since you also raised it, I'll change it to mIsSynchronous.
Flags: needinfo?(n.nethercote)
Assignee | ||
Comment 22•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9b2934ac8606387e71e1f248613d4e32000cea7 Bug 1351963 (part 2, attempt 2) - Move StreamSamplesAndMarkers() out of ThreadInfo. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/6d446e9be0749c744f80370d63287e6316d15766 Bug 1351963 (part 3, attempt 2) - Remove ThreadInfo from ProfilerBacktrace. r=mstange.
Assignee | ||
Comment 23•7 years ago
|
||
> > + ThreadInfo* info = new ThreadInfo(aName, Thread::GetCurrentId(),
>
> The effect of this is to replace one call to Thread::GetCurrentId() with
> two, right? Does that matter?
Yes. I don't think it matters. The result should always be the same, and it's not a performance concern because thread creation/registration is an infrequent operation.
Assignee | ||
Comment 24•7 years ago
|
||
> > static void
> > +FillInSample(TickSample* sample, ucontext_t* aContext)
>
> Could aContext be a ucontext_t& or even a const ucontext_t&?
> IIUC we're not modifying it.
It comes from PopulateContext(), where it must be a pointer because on Mac it's null. And we assign it to TickSample::mContext, which must be pointer because it's null to begin with. So it's possible, but we'd end up going back and forth between pointer and reference forms. So I'll just keep it as a pointer.
Assignee | ||
Comment 25•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bed8d9d6d6895cf35e4eaf9c12f3339a3a254025 Bug 1351963 (part 4) - Make the LastSample argument to addTagThreadId optional. r=jseward. https://hg.mozilla.org/integration/mozilla-inbound/rev/281d225824a321f2f5a3cb94839146d22b5079e9 Bug 1351963 (part 5) - Improve TickSample. r=jseward. https://hg.mozilla.org/integration/mozilla-inbound/rev/72eac9880a00321b51bffa4c8c3f3ece491c3b2a Bug 1351963 (part 6) - Remove ThreadInfo from TickSample. r=jseward. https://hg.mozilla.org/integration/mozilla-inbound/rev/4b21b0a87ce34817928d1d0d539e9b6678e90b44 Bug 1351963 (part 7) - Factor out repeated thread-finding code. r=jseward. https://hg.mozilla.org/integration/mozilla-inbound/rev/ff57919b70b9273cd36d1811cdbe8f29a4f10773 Bug 1351963 (part 8) - Simplify PseudoStack ownership. r=jseward. https://hg.mozilla.org/integration/mozilla-inbound/rev/9dcefc9cd349f546492fc79caeaf5315d3912225 Bug 1351963 (part 9) - Improve SetSampleContext(). r=jseward.
Comment 26•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d9b2934ac860 https://hg.mozilla.org/mozilla-central/rev/6d446e9be074 https://hg.mozilla.org/mozilla-central/rev/bed8d9d6d689 https://hg.mozilla.org/mozilla-central/rev/281d225824a3 https://hg.mozilla.org/mozilla-central/rev/72eac9880a00 https://hg.mozilla.org/mozilla-central/rev/4b21b0a87ce3 https://hg.mozilla.org/mozilla-central/rev/ff57919b70b9 https://hg.mozilla.org/mozilla-central/rev/9dcefc9cd349
You need to log in
before you can comment on or make changes to this bug.
Description
•