1362814 - Some precursors for adding a Gecko Profiler label to patched_LdrLoadDll()

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[(no longer active)]

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

Comment 1

[(no longer active)]

Attachment: Add a Gecko Profiler label to patched_LdrLoadDll()

Comment 2

[Markus Stange [:mstange]]

Comment on attachment 8865169 [details] [diff] [review]
Add a Gecko Profiler label to patched_LdrLoadDll()

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

Good idea

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fec6894255e
Add a Gecko Profiler label to patched_LdrLoadDll(); r=mstange

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for breaking Windows builds:

https://hg.mozilla.org/integration/mozilla-inbound/rev/31b8a9e5d1307eeb3990fb00904c115fbdb3d2b1

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1fec6894255e3298f36a2a6b91d48b2f96d07b11&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=97153955&repo=mozilla-inbound

15:32:00     INFO -  z:\build\build\src\obj-firefox\dist\include\nsStringFwd.h(15): fatal error C1189: #error:  Internal string headers are not available from external-linkage code.
15:32:00     INFO -  z:/build/build/src/config/rules.mk:1061: recipe for target 'WindowsDllBlocklist.obj' failed
15:32:00     INFO -  mozmake.EXE[5]: *** [WindowsDllBlocklist.obj] Error 2

Comment 5

[(no longer active)]

Thankfully the MainThreadUtils.h #include in GeckoProfiler.h that causes this is actually not needed!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aefe9ec739e1
Part 0: Remove an unneeded inclusion from GeckoProfiler.h
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd40f51b033f
Part 1: Add a Gecko Profiler label to patched_LdrLoadDll(); r=mstange

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for Windows bustage in nsStringFwd.h:

https://hg.mozilla.org/integration/mozilla-inbound/rev/284d41dc240f0dea28af10a52327d0e6a99f62cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4ca9b55771e84653d6ba21bf2701b33995fc026

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bd40f51b033f87a4eb2af2d47e18f14f3f3db3c0&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=97183728&repo=mozilla-inbound

14:46:53     INFO -  Note: including file:     c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\src\obj-firefox\dist\include\nsStringFwd.h
14:46:53     INFO -  c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\src\obj-firefox\dist\include\nsStringFwd.h(15): fatal error C1189: #error:  Internal string headers are not available from external-linkage code.
14:46:53     INFO -  c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/src/obj-firefox/_virtualenv/Scripts/python.exe c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/src/config/nsinstall.py -t -m 644 'TestCountZeroes.exe' '../../dist/cppunittests'
14:46:53     INFO -  mozmake.EXE[5]: Leaving directory 'c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/src/obj-firefox/layout/media'
14:46:53     INFO -  c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/src/config/rules.mk:1061: recipe for target 'WindowsDllBlocklist.obj' failed
14:46:53     INFO -  mozmake.EXE[5]: *** [WindowsDllBlocklist.obj] Error 2

Comment 8

[(no longer active)]

I don't understand, I *did* a try push and it came out green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=63f0316f051b37866171ed56ad9c509f3ba09c43

Comment 9

[(no longer active)]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #8)
> I don't understand, I *did* a try push and it came out green:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=63f0316f051b37866171ed56ad9c509f3ba09c43

Sigh, looks like that was pushed on top of m-c by mistake.  :-(  Sorry about that!

Comment 10

[(no longer active)]

Markus, doing this requires inlining profiler_call_enter and profiler_call_exit, because they need to be callable from mozglue.dll, which requires inlining <https://searchfox.org/mozilla-central/rev/8b70b0a5038ef9472fe7c53e04a70c978bb06aed/tools/profiler/core/platform.cpp#479> and which means its dependencies (PSLockRef and ThreadInfo) will need to be exported publicly.  Are you OK with this?  This ended up as a rather invasive change.  :-/

I have several patches so far that get me to a linker error.

Comment 11

[(no longer active)]

Attachment: Part 1: Remove an unneeded inclusion from GeckoProfiler.h

Comment 12

[(no longer active)]

Attachment: Part 2: Move GeckoProfilerReporter to its own header

This allows us to get rid of the nsIMemoryReporter.h inclusion in GeckoProfiler.h.
That brings XPCOM string headers with it.

Comment 13

[(no longer active)]

Attachment: Part 3: Inline profiler_call_enter and profiler_call_begin

Otherwise calling them from within mozglue.dll will fail.

Comment 14

[(no longer active)]

Attachment: Part 4: Add a Gecko Profiler label to patched_LdrLoadDll()

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

Comment 15

[Markus Stange [:mstange]]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #10)
> which means its dependencies (PSLockRef and ThreadInfo) will
> need to be exported publicly.  Are you OK with this?  This ended up as a
> rather invasive change.  :-/

Sounds like this is what njn meant in bug 1359000 comment 4 with "This change really makes the following patches *much* simpler."

Comment 16

[Markus Stange [:mstange]]

Your patches seem great to me. njn, what was the problem with including PseudoStack.h in GeckoProfiler.h?

Comment 17

[Markus Stange [:mstange]]

Oh, nevermind, these patches don't compile.

Comment 18

[(no longer active)]

Comment on attachment 8865310 [details] [diff] [review]
Part 1: Remove an unneeded inclusion from GeckoProfiler.h

This one can land (since the overall patchset may get stalled for a while)

Comment 19

[(no longer active)]

Comment on attachment 8865311 [details] [diff] [review]
Part 2: Move GeckoProfilerReporter to its own header

So can this.

Comment 20

[(no longer active)]

So how can we make progress here?

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/656f2a934ccd
Part 1: Remove an unneeded inclusion from GeckoProfiler.h; r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/27fb70fdc744
Part 2: Move GeckoProfilerReporter to its own header; r=mstange

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/656f2a934ccd
https://hg.mozilla.org/mozilla-central/rev/27fb70fdc744

Comment 23

[Markus Stange [:mstange]]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #20)
> So how can we make progress here?

Bug 1362894 has patches to make profiler_call_enter/exit inline again. Does that help with the build failures?

Comment 24

[(no longer active)]

(In reply to Markus Stange [:mstange] from comment #23)
> (In reply to :Ehsan Akhgari (super long backlog, slow to respond) from
> comment #20)
> > So how can we make progress here?
> 
> Bug 1362894 has patches to make profiler_call_enter/exit inline again. Does
> that help with the build failures?

Let me do another try push to find out.

Comment 25

[(no longer active)]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #24)
> (In reply to Markus Stange [:mstange] from comment #23)
> > (In reply to :Ehsan Akhgari (super long backlog, slow to respond) from
> > comment #20)
> > > So how can we make progress here?
> > 
> > Bug 1362894 has patches to make profiler_call_enter/exit inline again. Does
> > that help with the build failures?
> 
> Let me do another try push to find out.

The try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdee917871f8692b606df3b3823d3a88ecb496b2

The error:

c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\obj-firefox\dist\include\GeckoProfiler.h(407): error C2492: 'sPseudoStack': data with thread storage duration may not have dll interface [log…]

Comment 26

[(no longer active)]

I think I've fought the battle of making this macro work outside of libxul enough in this bug.  It's time to file another bug to actually make this work properly first, and once (if) someone does that we can land this patch.

Comment 27

[Nicholas Nethercote [inactive]]

Parts 1 and 2 have landed. Part 3 is no longer needed because bug 1362894 did the same thing. And part 4 is no longer needed because I'm going to do it as the last part of bug 1370329.