1359000 - Overhaul ThreadInfo and PseudoStack

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Nicholas Nethercote [inactive]]

There is room for improvement in how ThreadInfo and PseudoStack work.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove unused method PseudoStack::push

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Tweak manual polling of profiler threads

This patch moves the manual polling up into the preceding loops, which is a
better place for it.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Rename some "stack" variables as "pseudoStack"

"pseudoStack" more closely matches the type name, and is more specific.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - De-inline profiler_call_{enter,exit}

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.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Introduce TLSInfo

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.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Split off RacyThreadInfo from PseudoStack

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.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Move mContext and mJSSampling from RacyThreadInfo to ThreadInfo

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.

Comment 8

[Markus Stange [:mstange]]

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.

Comment 9

[Markus Stange [:mstange]]

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?

Comment 10

[Markus Stange [:mstange]]

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?

Comment 11

[Nicholas Nethercote [inactive]]

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

Comment 12

[Nicholas Nethercote [inactive]]

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

Comment 13

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af4f0150320bb7139a0a5ec84c8a6df49a5d671a
Bug 1359000 (part 1) - Remove unused method PseudoStack::push. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/27ac1ce3ebc8f9a10a84c4c044c50789eaf5333b
Bug 1359000 (part 2) - Tweak manual polling of profiler threads. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/572e82ea517b893af4733c2b67a974ed2fa62f12
Bug 1359000 (part 3) - Rename some "stack" variables as "pseudoStack". r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f93595edab7d54ec4f39308a4d06db85865dd371
Bug 1359000 (part 4) - De-inline profiler_call_{enter,exit}. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/427f09c04b04d5cb1279f05146589539cb7a59f7
Bug 1359000 (part 5) - Introduce TLSInfo. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c13ffe0d6aaf8c7ad57043f4cb66f65e211dca1e
Bug 1359000 (part 6) - Split off RacyThreadInfo from PseudoStack. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/658b857c35f848ad8d9c428c586cce875f9434a3
Bug 1359000 (part 7) - Move mContext and mJSSampling from RacyThreadInfo to ThreadInfo. r=mstange.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/af4f0150320b
https://hg.mozilla.org/mozilla-central/rev/27ac1ce3ebc8
https://hg.mozilla.org/mozilla-central/rev/572e82ea517b
https://hg.mozilla.org/mozilla-central/rev/f93595edab7d
https://hg.mozilla.org/mozilla-central/rev/427f09c04b04
https://hg.mozilla.org/mozilla-central/rev/c13ffe0d6aaf
https://hg.mozilla.org/mozilla-central/rev/658b857c35f8