Closed Bug 1359000 Opened 4 years ago Closed 4 years ago

Overhaul ThreadInfo and PseudoStack

Categories

(Core :: Gecko Profiler, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

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

References

Details

Attachments

(7 files)

There is room for improvement in how ThreadInfo and PseudoStack work.
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
This patch moves the manual polling up into the preceding loops, which is a
better place for it.
Attachment #8860872 - Flags: review?(mstange)
"pseudoStack" more closely matches the type name, and is more specific.
Attachment #8860873 - Flags: review?(mstange)
Markus, you reluctantly r+'d a similar patch in bug 1347795. I'm hopeful that
the performance impact of this will be minimal and that other later changes
will more than make up for any degradation here. This change really makes the
following patches *much* simpler.
Attachment #8860874 - Flags: review?(mstange)
Currently a reference to each thread's PseudoStack is stored in tlsPseudoStack.
This patch changes the TLS reference to refer to the enclosing ThreadInfo
instead. This allows profiler_clear_js_context() to access the current thread's
ThreadInfo via TLs, rather than searching with FindLiveThreadInfo().

The patch also encapsulates the TLS within a new class called TLSInfo. This
class allows access to the PseudoStack without protection from gPSMutex, but
access to the enclosing ThreadInfo requires a PSLockRef. This maintains the
current access regime.
Attachment #8860879 - Flags: review?(mstange)
PseudoStack is misnamed: it contains the pseudo-stack plus other stuff that is
accessed via TLS.

This patch moves the non-pseudo-stack parts of PseudoStack into a new type
called RacyThreadInfo, which is a subclass of PseudoStack. The patch also
capitalizes the first letter of the names of methods that it moves.

This means that PseudoStack is now accurately named. Also non-pseudo-stack
parts are now no longer visible outside the profiler, which is nice.
Attachment #8860880 - Flags: review?(mstange)
None of the accesses to these fields occur in hot operations, so it's
reasonable to do them with gPSMutex held. As a result, mJSSampling doesn't need
to be Atomic<>, and mContext's lack of Atomic-ness is no longer a cause for
concern.

This required adding an extra field, mJSContext, to TickSample.
Attachment #8860881 - Flags: review?(mstange)
Depends on: 1358074
Blocks: 1359007
Attachment #8860871 - Flags: review?(mstange) → review+
Attachment #8860872 - Flags: review?(mstange) → review+
Attachment #8860873 - Flags: review?(mstange) → review+
Blocks: 1123754
No longer blocks: 1359007
Comment on attachment 8860874 [details] [diff] [review]
(part 4) - De-inline profiler_call_{enter,exit}

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

I'm still very reluctant to r+ this, but you make a convincing argument.

And the fact that one needs to recompile the world when touching PseudoStack.h has long been a thorn in my side.
Attachment #8860874 - Flags: review?(mstange) → review+
Attachment #8860879 - Flags: review?(mstange) → review+
Comment on attachment 8860880 [details] [diff] [review]
(part 6) - Split off RacyThreadInfo from PseudoStack

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

::: tools/profiler/core/ThreadInfo.h
@@ +18,5 @@
> +
> +// This class contains the info for a single thread that is accessible without
> +// protection from gPSMutex in platform.cpp. Because there is no external
> +// protection against data races, it must provide internal protection. Hence
> +// the "Racy" prefix.

Aren't things that can be accessed from multiple threads without protection usually called threadsafe?

@@ +54,5 @@
> +
> +    return n;
> +  }
> +
> +  void AddPendingMarker(const char* aMarkerStr, ProfilerMarkerPayload* aPayload,

This is only called on-thread, I think. Add a comment?
Attachment #8860880 - Flags: review?(mstange) → review+
Attachment #8860881 - Flags: review?(mstange) → review+
Comment on attachment 8860880 [details] [diff] [review]
(part 6) - Split off RacyThreadInfo from PseudoStack

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

::: tools/profiler/core/ThreadInfo.cpp
@@ +23,5 @@
>                         void* aStackTop)
>    : mName(strdup(aName))
>    , mThreadId(aThreadId)
>    , mIsMainThread(aIsMainThread)
> +  , mRacyInfo(mozilla::WrapNotNull(new RacyThreadInfo()))

Could this be a regular field now?
> > +// This class contains the info for a single thread that is accessible without
> > +// protection from gPSMutex in platform.cpp. Because there is no external
> > +// protection against data races, it must provide internal protection. Hence
> > +// the "Racy" prefix.
> 
> Aren't things that can be accessed from multiple threads without protection
> usually called threadsafe?

I have seen that once or twice and found it highly confusing! That naming scheme seems entirely backwards to me. "Racy" suggests "be careful!" which is appropriate. "Threadsafe" suggests "you are protected"...
> > +  , mRacyInfo(mozilla::WrapNotNull(new RacyThreadInfo()))
> 
> Could this be a regular field now?

Almost. The field comment explains why it's a pointer:

> // The thread's RacyThreadInfo. This is an owning pointer. It could be an
> // inline member, but we don't do that because RacyThreadInfo is quite large
> // (due to the PseudoStack within it), and we have ThreadInfo vectors and so
> // we'd end up wasting a lot of space in those vectors for excess elements.
> mozilla::NotNull<RacyThreadInfo*> mRacyInfo;

When we change PseudoStack from an array to a linked list, then that's a good time to make this an inline member instead of a pointer.
Depends on: 1362894
You need to log in before you can comment on or make changes to this bug.