Closed Bug 1351963 Opened 8 years ago Closed 8 years ago

Get rid of fake ThreadInfos in profiler_get_backtrace()

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

Details

Attachments

(9 files)

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.
Attached patch (part 1) - Tidy up ThreadInfo — — Splinter Review
It doesn't need any virtual methods, nor the FRIEND_TEST declarations.
Attachment #8853236 - Flags: review?(mstange)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
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)
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)
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)
Attached patch (part 5) - Improve TickSample — — Splinter Review
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)
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)
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)
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)
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)
Attachment #8853236 - Flags: review?(mstange) → review+
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+
Attachment #8853238 - Flags: review?(mstange) → review+
> > + // 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.
Oh! The argument is actually a JSContext, not a PseudoStack. I will reword it again :)
Attachment #8853236 - Flags: checkin+
Attachment #8853237 - Flags: checkin+
Attachment #8853238 - Flags: checkin+
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)
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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 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+
Attachment #8853241 - Flags: review?(jseward) → review+
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+
Attachment #8853243 - Flags: review?(jseward) → review+
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+
> 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)
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.
> > + 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.
> > 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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: