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
3 months ago
2 months 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

3 months 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

3 months 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

3 months ago
No longer blocks: 1362814
Depends on: 1362814
(Assignee)

Updated

3 months 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

3 months 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

3 months 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

3 months 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+

Updated

2 months ago
Attachment #8875108 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 9

2 months 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

2 months 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: 2 months 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.