1347795 - PseudoStack cleanups

Status: RESOLVED WONTFIX

(Core :: Gecko Profiler, enhancement)

Description

[Nicholas Nethercote [inactive]]

I have some PseudoStack cleanups in preparation.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Tweak profiler_get_pseudo_stack()

This patch tweaks it so that we get access to the stack entries directly,
without having to see the PseudoStack type. This will allow us to de-export
PseudoStack from the profiler.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - De-export PseudoStack.h from the profiler

This requires moving tlsPseudoStack and the definitions of
profiler_call_{enter,exit}() out of GeckoProfiler.h. But that also means tht
ProfilerState and gPS can be moved similarly.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - De-export PseudoStack.h from the profiler

This requires moving tlsPseudoStack and the definitions of
profiler_call_{enter,exit}() out of GeckoProfiler.h. But that also means that
ProfilerState and gPS can be moved similarly.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Rename PseudoStack as TLSInfo and break into four pieces

PseudoStack is really the pseudo-stack plus three other unrelated pieces. The
one thing these four pieces have in common is that they are all stored in TLS.

So this patch renames PseudoStack as TLSInfo and introduces four new classes
from which TLSInfo is composed. (One of those new classes is called
PseudoStack, and contains just the pseudo-stack.)

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 8848987 [details] [diff] [review]
(part 3) - Rename PseudoStack as TLSInfo and break into four pieces

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

Hmm, now I'm wondering if it would be better to merge ThreadInfo and TLSInfo. So I'll cancel this review request for now.

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8848956 [details] [diff] [review]
(part 2) - De-export PseudoStack.h from the profiler

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

Okay, I suppose. Talos will hopefully tell us whether we're making anything noticeably worse with this.

::: tools/profiler/public/GeckoProfiler.h
@@ +318,5 @@
>  // Returns a handle to pass on exit. This can check that we are popping the
>  // correct callstack. Operates the same whether the profiler is active or not.
> +void* profiler_call_enter(const char* aInfo,
> +                          js::ProfileEntry::Category aCategory,
> +                          void *aFrameAddress, bool aCopy, uint32_t line);

Un-inlining this doesn't really get us closer to bug 1348402 :(

Comment 7

[Nicholas Nethercote [inactive]]

A rough roadmap here:

- I want to de-export PseudoStack, because it's barely used outside the profiler, as parts 1 and 2 show.

- We store per-thread data in two ways: ThreadInfo, which is stored in a vector, and PseudoStack, which is stored in TLS. Each ThreadInfo also has a pointer to each PseudoStack. In some places we do something with every entry in the vector, and in some places we just search the vector for a particular ThreadInfo. So it probably makes sense to just merge ThreadInfo and PseudoStack into a single class, which would then be accessible via the vector (when iterating over all threads) and directly via TLS (when dealing with a single thread).

Comment 8

[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 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8860876 [details] [diff] [review]
(part 5) - Introduce TLSInfo

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

Sorry, that patch was meant to go on bug 1359000.

Comment 10

[Nicholas Nethercote [inactive]]

Bug 1359000 has superseded this bug.