Closed Bug 1362814 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: General, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(2 files, 3 obsolete files)

This allows us to easily identify the name of the DLL being loaded
in a profile.
Attachment #8865169 - Flags: review?(mstange)
Assignee: nobody → ehsan
Comment on attachment 8865169 [details] [diff] [review]
Add a Gecko Profiler label to patched_LdrLoadDll()

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

Good idea
Attachment #8865169 - Flags: review?(mstange) → review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fec6894255e
Add a Gecko Profiler label to patched_LdrLoadDll(); r=mstange
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
Flags: needinfo?(ehsan)
Thankfully the MainThreadUtils.h #include in GeckoProfiler.h that causes this is actually not needed!
Flags: needinfo?(ehsan)
Pushed by eakhgari@mozilla.com:
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
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
Flags: needinfo?(ehsan)
I don't understand, I *did* a try push and it came out green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=63f0316f051b37866171ed56ad9c509f3ba09c43
Flags: needinfo?(ehsan)
(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!
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.
Flags: needinfo?(mstange)
Attachment #8865169 - Attachment is obsolete: true
This allows us to get rid of the nsIMemoryReporter.h inclusion in GeckoProfiler.h.
That brings XPCOM string headers with it.
Otherwise calling them from within mozglue.dll will fail.
This allows us to easily identify the name of the DLL being loaded
in a profile.
(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."
Your patches seem great to me. njn, what was the problem with including PseudoStack.h in GeckoProfiler.h?
Flags: needinfo?(mstange) → needinfo?(n.nethercote)
Oh, nevermind, these patches don't compile.
Flags: needinfo?(n.nethercote)
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)
Attachment #8865310 - Flags: review?(mstange)
Comment on attachment 8865311 [details] [diff] [review]
Part 2: Move GeckoProfilerReporter to its own header

So can this.
Attachment #8865311 - Flags: review?(mstange)
Attachment #8865310 - Flags: review?(mstange) → review+
Attachment #8865311 - Flags: review?(mstange) → review+
Keywords: leave-open
So how can we make progress here?
Pushed by eakhgari@mozilla.com:
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
(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?
(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.
(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…]
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.
Depends on: 1370329
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.
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1370329
No longer blocks: 1362382
No longer depends on: 1370329
Summary: Add a Gecko Profiler label to patched_LdrLoadDll() → Some precursors for adding a Gecko Profiler label to patched_LdrLoadDll()
Attachment #8865312 - Attachment is obsolete: true
Attachment #8865313 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.