1370329 - Allow using PROFILER_LABEL macros from outside libxul, and do so from patched_LdrLoadDll()

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[(no longer active)]

These macros right now aren't usable outside of libxul due to linking errors on Windows.  See bug 1362814.  We'd like to use them from mozglue.dll at least.

Comment 1

[Markus Stange [:mstange]]

Nick, could you look into this?

Comment 2

[Nicholas Nethercote [inactive]]

Sure, I'll take a look.

Comment 3

[(no longer active)]

Thank you!  Fixing bug 1362814 is a huge help to the ongoing statrtup optimization work.  :-)

Comment 4

[Nicholas Nethercote [inactive]]

The basic problem is that mozglue (where you want to add the PROFILER_LABEL in bug 1362814) can't have a dependency on libxul (which holds the profiler).

We'll need to move sPseudoStack somewhere else. Possibly into MFBT. Assuming that works, we'll need to be careful that this doesn't make sPseudoStack accesses more expensive within libxul.

Comment 5

[Markus Stange [:mstange]]

If that's too much work, as a shorter-term workaround we could make libxul register a callback function with mozglue and hope that LoadDLL is the only mozglue function that we want to instrument.

mozglue:

typedef std::function<NTSTATUS()> CallLoadDllFunc;
typedef (NTSTATUS *LoadDllWrapperFunc)(const char* aDllName, CallLoadDllFunc& aWrappedFunction);

NS_RegisterLoadDllWrapper(LoadDllWrapperFunc* aWrapper);


libxul:

static NTSTATUS
AnnotateLoadDllWithProfilerLabel(const char* aDllName, CallLoadDllFunc& aWrappedFunction)
{
  PROFILER_LABEL_DYNAMIC("WindowsDllBlocklist", "patched_LdrLoadDll",
                         js::ProfileEntry::Category::OTHER, aDllName);
  return aWrappedFunction();
}

  // ...
  // somewhere during startup:
  NS_RegisterLoadDllWrapper(&AnnotateLoadDllWithProfilerLabel);

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Clean up the profiler's RAII classes

This patch does the following renamings, which increase consistency.

- GeckoProfilerInitRAII -> AutoProfilerInit
- GeckoProfilerThread{Sleep,Wake}RAII -> AutoProfilerThread{Sleep,Wake}
- GeckoProfilerTracingRAII -> AutoProfilerTracing
- AutoProfilerRegister -> AutoProfilerRegisterThread
- ProfilerStackFrameRAII -> AutoProfilerLabel
- nsJSUtils::mProfilerRAII -> nsJSUtils::mAutoProfilerLabel

Plus a few other minor ones (e.g. local variables).

The patch also add MOZ_GUARD_OBJECT macros to all the profiler RAII classes
that lack them, and does some minor whitespace reformatting.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Add support for profiler labels in mozglue

Profiler labels can't currently be used in mozglue, because the profiler's code
is in libxul, and mozglue cannot depend on libxul.

This patch addresses this by basically duplicating AutoProfilerLabel in
mozglue. libxul passes two callback functions to mozglue to do the actual
pushing/popping of labels.

It's an annoying amount of machinery, but it is unavoidable if we want to use
profiler labels within mozglue.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Add a Gecko Profiler label to patched_LdrLoadDll()

This lets us easily identify the name of the DLL being loaded in a profile.

Comment 9

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48c1d3b22bc0ef4374b87aa98904be3586e3d153
Bug 1370329 (part 1) - Clean up the profiler's RAII classes. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/579ab9b515d6ce9625d75a28893da424a1f8c289
Bug 1370329 (part 2) - Add support for profiler labels in mozglue. r=mstange,glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/93c502ef3313f296be5a91e7bea11cc3d443ae20
Bug 1370329 (part 3) - Add a Gecko Profiler label to patched_LdrLoadDll(). r=mstange.

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/48c1d3b22bc0
https://hg.mozilla.org/mozilla-central/rev/579ab9b515d6
https://hg.mozilla.org/mozilla-central/rev/93c502ef3313