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

RESOLVED FIXED in Firefox 56

Status

()

Core
Gecko Profiler
RESOLVED FIXED
19 days ago
11 days ago

People

(Reporter: Ehsan, Assigned: njn)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(3 attachments)

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.
Nick, could you look into this?
Flags: needinfo?(n.nethercote)
(Assignee)

Comment 2

19 days ago
Sure, I'll take a look.
Assignee: nobody → n.nethercote
Flags: needinfo?(n.nethercote)
Thank you!  Fixing bug 1362814 is a huge help to the ongoing statrtup optimization work.  :-)
(Assignee)

Comment 4

19 days ago
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.
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);
(Assignee)

Updated

18 days ago
No longer blocks: 1362814
Depends on: 1362814
(Assignee)

Updated

18 days ago
Summary: Allow using PROFILER_LABEL macros from outside libxul → Allow using PROFILER_LABEL macros from outside libxul, and do so from patched_LdrLoadDll()
(Assignee)

Comment 6

18 days ago
Created attachment 8875106 [details] [diff] [review]
(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.
Attachment #8875106 - Flags: review?(mstange)
(Assignee)

Comment 7

18 days ago
Created attachment 8875108 [details] [diff] [review]
(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.
Attachment #8875108 - Flags: review?(mstange)
Attachment #8875108 - Flags: review?(mh+mozilla)
(Assignee)

Comment 8

18 days ago
Created attachment 8875109 [details] [diff] [review]
(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.
Attachment #8875109 - Flags: review?(mstange)
Attachment #8875108 - Flags: review?(mstange) → review+
Attachment #8875106 - Flags: review?(mstange) → review+
Attachment #8875109 - Flags: review?(mstange) → review+
Attachment #8875108 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 9

12 days ago
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

11 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/48c1d3b22bc0
https://hg.mozilla.org/mozilla-central/rev/579ab9b515d6
https://hg.mozilla.org/mozilla-central/rev/93c502ef3313
Status: NEW → RESOLVED
Last Resolved: 11 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.