1351963 - Get rid of fake ThreadInfos in profiler_get_backtrace()

Status: RESOLVED FIXED

Description

[Nicholas Nethercote [:njn]]

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.

Comment 1

[Nicholas Nethercote [:njn]]

Created attachment 8853236 [details] [diff] [review]
(part 1) - Tidy up ThreadInfo

It doesn't need any virtual methods, nor the FRIEND_TEST declarations.

Comment 2

[Nicholas Nethercote [:njn]]

Created attachment 8853237 [details] [diff] [review]
(part 2) - Move StreamSamplesAndMarkers() out of ThreadInfo

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.

Comment 3

[Nicholas Nethercote [:njn]]

Created attachment 8853238 [details] [diff] [review]
(part 3) - Remove ThreadInfo from ProfilerBacktrace

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().

Comment 4

[Nicholas Nethercote [:njn]]

Created attachment 8853239 [details] [diff] [review]
(part 4) - Make the LastSample argument to addTagThreadId optional

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.

Comment 5

[Nicholas Nethercote [:njn]]

Created attachment 8853240 [details] [diff] [review]
(part 5) - Improve TickSample

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.

Comment 6

[Nicholas Nethercote [:njn]]

Created attachment 8853241 [details] [diff] [review]
(part 6) - Remove ThreadInfo from TickSample

This avoids the need for the fake ThreadInfo in profiler_get_backtrace(). It
requires adding a few extra fields to TickSample.

Comment 7

[Nicholas Nethercote [:njn]]

Created attachment 8853242 [details] [diff] [review]
(part 7) - Factor out repeated thread-finding code

The patch also adds a MOZ_RELEASE_ASSERT in profiler_unregister_thread() for
the case where the ThreadInfo isn't found, which is informative.

Comment 8

[Nicholas Nethercote [:njn]]

Created attachment 8853243 [details] [diff] [review]
(part 8) - Simplify PseudoStack ownership

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.

Comment 9

[Nicholas Nethercote [:njn]]

Created attachment 8853244 [details] [diff] [review]
(part 9) - Improve SetSampleContext()

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.

Comment 10

[Markus Stange [:mstange]]

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.

Comment 11

[Nicholas Nethercote [:njn]]

> > +  // 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.

Comment 12

[Nicholas Nethercote [:njn]]

Oh! The argument is actually a JSContext, not a PseudoStack. I will reword it again :)

Comment 13

[Nicholas Nethercote [:njn]]

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.

Comment 14

[Carsten Book [:Tomcat]]

backed out part 2 & 3 in case this is causing leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=88226147&repo=mozilla-inbound

Comment 15

[Pulsebot]

Backout by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5af677c09ae7

Comment 17

[Julian Seward [:jseward]]

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|.

Comment 18

[Julian Seward [:jseward]]

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.

Comment 19

[Julian Seward [:jseward]]

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?

Comment 20

[Julian Seward [:jseward]]

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.

Comment 21

[Nicholas Nethercote [:njn]]

> 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.

Comment 22

[Nicholas Nethercote [:njn]]

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.

Comment 23

[Nicholas Nethercote [:njn]]

> > +  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.

Comment 24

[Nicholas Nethercote [:njn]]

> >  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.

Comment 25

[Nicholas Nethercote [:njn]]

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

[Carsten Book [:Tomcat]]

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