1127498 - Share one buffer between all threads, improve marker lifetime management, some code cleanup

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Markus Stange [:mstange]]

Attachment: profiler-changes

Making all threads write into the same profile buffer makes better use of the same amount of memory: Instead of having, for example, less than a second of main thread coverage plus minutes of compositor thread coverage, we'll have the same time range for all threads.

This patch does the following things:
 - Splits out the circular buffer management from ThreadProfile and moves it into a ProfileBuffer class, which is refcounted.
 - Creates one ProfileBuffer instance for the TableTicker and shares this buffer between all ThreadProfiles of the process. For SyncProfiles we create one instance in the SyncProfile and pass that to the ThreadProfile (its superclass). These two different ways of keeping the buffer alive are the reason I used refcounting.
 - TableTicker::InPlaceTick writes out a 'T' tag that contains the thread ID at the start of every captured sample. This is how we know what thread a sample in the buffer belongs to. This comes with an extremely small overhead compared to the previous state of things.
 - StoredMarkers are moved out of PendingMarkers into the ProfileBuffer. This fixes bug 1124368. So after the tick, the PseudoStack is free to go away as far as anything in the ProfileBuffer is concerned.
 - Expired stored markers are deleted by the SamplerThread after each sleep. This happens outside of a signal handler and means we're free to call destructors and release memory.
 - PendingMarkers and PendingUWTBuffers are merged into one ProfilerSignalSafeLinkedList<T> class.
 - "Jank-only" mode is removed, and with it the flush() and erase() functions.

Comment 1

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43af5a20d03b

Comment 2

[Markus Stange [:mstange]]

Attachment: profiler-changes

Had to remove some ThreadProfile::flush callers from UnwinderThread2.cpp. Try was very sad about those.

Comment 3

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e64f2decfb7

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 8556670 [details] [diff] [review]
profiler-changes

Review of attachment 8556670 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/ProfileEntry.h
@@ +76,5 @@
> +class ProfileBuffer {
> +public:
> +  NS_INLINE_DECL_REFCOUNTING(ProfileBuffer)
> +
> +  ProfileBuffer(int aEntrySize);

This needs an 'explicit'.

Comment 5

[Benoit Girard (:BenWa)]

Comment on attachment 8556670 [details] [diff] [review]
profiler-changes

Review of attachment 8556670 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/ProfileEntry.cpp
@@ +93,5 @@
>    // by looking through all the use points of TableTicker.cpp.
>    //   mTagMarker (ProfilerMarker*) m
>    //   mTagData   (const char*)  c,s
>    //   mTagPtr    (void*)        d,l,L,B (immediate backtrace), S(start-of-stack)
> +  //   mTagInt    (int)          n,f,y,T

T (thread id)

@@ +433,5 @@
> +  for (int readPos = (lastSampleStartPos + 1) % mEntrySize;
> +       readPos != mWritePos;
> +       readPos = (readPos + 1) % mEntrySize) {
> +    switch (mEntries[readPos].mTagName) {
> +      case 'T':

Optional: Maybe you should assert here that you don't see the start of a sample 'S' here? I don't think it's possible but it would have pretty bad results.

::: tools/profiler/PseudoStack.h
@@ -172,5 @@
>    T* mHead;
>    T* mTail;
>  };
>  
>  typedef ProfilerLinkedList<ProfilerMarker> ProfilerMarkerLinkedList;

I wonder if we should rename either the type or the variables that use this type to include the word Intrusive. A few times a year when I forget that we're using an intrusive list and I keep thinking we incorrectly allocate in the signal handler.

@@ +196,5 @@
>      }
>    }
>  
> +  // Insert an item into the list.
> +  // Must only be called from a single thread.

I find the term 'Must only be called from a single thread.' confusing.

Either we should define the object as having an owning thread and say that this method must be called from the owning thread, a stricter requirement, or we say that this can not be called concurrently, a weaker requirement.

@@ +199,5 @@
> +  // Insert an item into the list.
> +  // Must only be called from a single thread.
> +  // Must not be called while the list from accessList() is being accessed.
> +  // In the profiler, we ensure that by interrupting the profiled thread
> +  // (which is the one that calls insert()) until we're done reading the list

Optional:
I wonder if we should throw an assert to make sure that this is only called from the owning thread. We keep a debug only handle to the owning thread and make sure we're on that thread when this is called.

This would prevent us from accidentally sharing a single ProfilerSignalSafeLinkedList for all threads and break this comment. If we asserted for that in the code we would be safer.

::: tools/profiler/tests/gtest/ThreadProfileTest.cpp
@@ +7,5 @@
>  
>  #include "ProfileEntry.h"
>  
>  // Make sure we can initialize our ThreadProfile
>  TEST(ThreadProfile, Initialization) {

Maybe we could create a test for ProfileBuffer where we insert a bunch of tags like:

Thread 1 Sample Start <frame 1> <frame 2> Sample Start <frame 1> <frame 2>
+
Thread 2 Sample Start <frame 1> <frame 2> Sample Start <frame 1> <frame 2>

And then call StreamSamplesToJSObject. Should be easy to write a test for that.

Comment 6

[Markus Stange [:mstange]]

(In reply to Benoit Girard (:BenWa) from comment #5)
> @@ +433,5 @@
> > +  for (int readPos = (lastSampleStartPos + 1) % mEntrySize;
> > +       readPos != mWritePos;
> > +       readPos = (readPos + 1) % mEntrySize) {
> > +    switch (mEntries[readPos].mTagName) {
> > +      case 'T':
> 
> Optional: Maybe you should assert here that you don't see the start of a
> sample 'S' here? I don't think it's possible but it would have pretty bad
> results.

Actually, we expect the very first entry we encounter to be an 'S' entry. We look for 'T', the next one will be 'S', then the rest of the stack trace + meta information, and then the next 'T', at which we stop copying.
I could check that we don't encounter *two* 'S' entries between two 'T' entries, but that's probably not worth it.

> ::: tools/profiler/PseudoStack.h
> @@ -172,5 @@
> >    T* mHead;
> >    T* mTail;
> >  };
> >  
> >  typedef ProfilerLinkedList<ProfilerMarker> ProfilerMarkerLinkedList;
> 
> I wonder if we should rename either the type or the variables that use this
> type to include the word Intrusive. A few times a year when I forget that
> we're using an intrusive list and I keep thinking we incorrectly allocate in
> the signal handler.

ProfilerIntrusiveLinkedList + ProfilerMarkerIntrusiveLinkedList + ProfilerSignalSafeIntrusiveLinkedList?

> @@ +196,5 @@
> >      }
> >    }
> >  
> > +  // Insert an item into the list.
> > +  // Must only be called from a single thread.
> 
> I find the term 'Must only be called from a single thread.' confusing.
> 
> Either we should define the object as having an owning thread and say that
> this method must be called from the owning thread,

Yeah, that sounds good to me.

> Optional:
> I wonder if we should throw an assert to make sure that this is only called
> from the owning thread. We keep a debug only handle to the owning thread and
> make sure we're on that thread when this is called.

I'll tweak the comment in this patch and write a new patch that adds such an assertion.

> ::: tools/profiler/tests/gtest/ThreadProfileTest.cpp
> @@ +7,5 @@
> >  
> >  #include "ProfileEntry.h"
> >  
> >  // Make sure we can initialize our ThreadProfile
> >  TEST(ThreadProfile, Initialization) {
> 
> Maybe we could create a test for ProfileBuffer where we insert a bunch of
> tags like:
> 
> Thread 1 Sample Start <frame 1> <frame 2> Sample Start <frame 1> <frame 2>
> +
> Thread 2 Sample Start <frame 1> <frame 2> Sample Start <frame 1> <frame 2>
> 
> And then call StreamSamplesToJSObject. Should be easy to write a test for
> that.

I'll do that in a new patch but I'll land this one first.

Comment 7

[Markus Stange [:mstange]]

I added the 'explicit' and tweaked the comments. I did not add 'intrusive' anywhere.
https://hg.mozilla.org/integration/mozilla-inbound/rev/29ac4f012129

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/29ac4f012129