Closed
Bug 1403868
Opened 7 years ago
Closed 7 years ago
GeckoProfiler.h included in xpcom/threads if MOZ_INTERNAL_API is defined
Categories
(Core :: XPCOM, enhancement, P3)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: cpearce, Assigned: n.nethercote)
References
Details
Attachments
(5 files, 2 obsolete files)
(deleted),
image/svg+xml
|
Details | |
(deleted),
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mstange
:
review+
cpearce
:
feedback+
|
Details | Diff | Splinter Review |
We're including GeckoProfiler.h inside xpcom/threads code if MOZ_INTERAL_API. This is a problem for the effort to extract Gecko's media stack to a stand alone rust crate, as we'd like to avoid including the Gecko profiler in our crate.
There is already a MOZ_GECKO_PROFILER #define that we could use instead as an include guard, which would work fine for Firefox, and allow the gecko-media crate to exclude the profiler.
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
Why is including the Gecko profiler in your crate a problem? If MOZ_GECKO_PROFILER is not defined, it should compile to mostly no-ops, I thought.
Updated•7 years ago
|
Attachment #8913122 -
Flags: review?(n.nethercote)
Comment 3•7 years ago
|
||
I'd like njn to take a look as well, because he has put more thought into how things are supposed to work if MOZ_GECKO_PROFILER is not defined.
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #2)
> Why is including the Gecko profiler in your crate a problem? If
> MOZ_GECKO_PROFILER is not defined, it should compile to mostly no-ops, I
> thought.
That's correct. You can see from tools/profiler/moz.build that when MOZ_GECKO_PROFILER is not defined, no .cpp files are built, and only three .h files are exported. At the very least AUTO_PROFILER_LABEL needs to be defined because that is used all over the codebase without a guard.
Indeed, you can see in nsTimerImpl.cpp that you've added guards for the `#include "GeckoProfiler.h"` and also for the AUTO_PROFILER_LABEL. This is setting us up for a bad time, because every time someone adds a new AUTO_PROFILER_LABEL they have to ask "is this file in the media stack?" -- which I think that's a non-trivial question -- and if the answer is "yes" they need to guard it, otherwise they don't. So I don't think this patch is acceptable.
Does that make sense?
The exact functioning of non-MOZ_GECKO_PROFILER builds has not received much attention because it's only been relevant for non-Tier-1 platforms up until now. But if it's now a more important configuration, I can try next week to reduce the amount of stuff in those three exported .h files to an absolute minimum. Would that be helpful?
Flags: needinfo?(cpearce)
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8913122 [details]
Bug 1403868 - Only include GeckoProfiler.h in xpcom/threads ifdef MOZ_GECKO_PROFILER.
https://reviewboard.mozilla.org/r/184542/#review189924
Attachment #8913122 -
Flags: review?(n.nethercote) → review-
Comment 6•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #4)
> Indeed, you can see in nsTimerImpl.cpp that you've added guards for the
> `#include "GeckoProfiler.h"` and also for the AUTO_PROFILER_LABEL. This is
> setting us up for a bad time, because every time someone adds a new
> AUTO_PROFILER_LABEL they have to ask "is this file in the media stack?" --
> which I think that's a non-trivial question -- and if the answer is "yes"
> they need to guard it, otherwise they don't. So I don't think this patch is
> acceptable.
I'd like to amplify this: a year or two ago, we tried to make a standalone WebRTC library implementation that liberally sprinkled #ifdefs all over xpcom. It was non-trivial to figure out whether you might be breaking that standalone thing, especially as, IIRC, there were no automation builds for it. From what little I have seen, the standalone media stack work has avoided ifdeffery, and it would be fantastic if the work could continue in that vein.
Reporter | ||
Comment 7•7 years ago
|
||
Basically our issue with GeckoProfiler.h is that it includes some JS headers, and we're trying to avoid that in order to simplify our include graph.
The include graph of JS headers included by GeckoProfiler.h is shallow (see attached), and while it's not a deal-breaker to include headers in the gecko-media crate build for symbols that we don't actually link against, I'd prefer to not make exceptions to the "no JS" rule unless we really need to.
We could work around this on our side by having a fake GeckoProfiler.h which #defines no-op or empty implementation of the symbols that Gecko uses. We've already done this for a few symbols, but it's a headache for some classes which are changing interface frequently.
Flags: needinfo?(cpearce)
Assignee | ||
Comment 8•7 years ago
|
||
Thank you for the extra info. I'll take a look at this on Monday, see if I can shrink GeckoProfiler.h down in the non-MOZ_GECKO_PROFILER case.
Assignee: cpearce → n.nethercote
Assignee | ||
Comment 9•7 years ago
|
||
We can use |this| instead, as is already done in AutoProfilerRegisterThread().
Attachment #8914652 -
Flags: review?(mstange)
Assignee | ||
Comment 10•7 years ago
|
||
This patch does the following.
- Makes the TracingKind argument non-optional.
- Puts the UniqueProfilerBacktrace argument last in the second variant.
- Reorders AutoProfilerTracing to match the order of the profiler_tracing()
declarations.
Attachment #8914653 -
Flags: review?(mstange)
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #8914654 -
Flags: review?(mstange)
Assignee | ||
Comment 12•7 years ago
|
||
Currently the Gecko Profiler defines a moderate amount of stuff when
MOZ_GECKO_PROFILER is undefined. It also #includes various headers, including
JS ones. This is making it difficult to separate Gecko's media stack for
inclusion in Servo.
This patch greatly simplifies how things are exposed. The starting point is:
- GeckoProfiler.h can be #included unconditionally;
- everything else from the profiler must be guarded by MOZ_GECKO_PROFILER.
In practice this introduces way too many #ifdefs, so the patch loosens it by
adding no-op macros for a number of the most common operations.
The net result is that #ifdefs and macros are used a bit more, but almost
nothing is exposed in non-MOZ_GECKO_PROFILER builds (including
ProfilerMarkerPayload.h and GeckoProfiler.h), and understanding what is exposed
is much simpler than before.
Note also that in BHR, ThreadStackHelper is now entirely absent in
non-MOZ_GECKO_PROFILER builds.
Attachment #8914655 -
Flags: review?(mstange)
Assignee | ||
Comment 13•7 years ago
|
||
cpearce: in non-MOZ_GECKO_PROFILER builds, GeckoProfiler.h now defines just a handful of no-op macros and includes no other files. That should suffice for your purposes, if I've understood correctly.
Assignee | ||
Updated•7 years ago
|
Attachment #8914655 -
Flags: feedback?(cpearce)
Assignee | ||
Updated•7 years ago
|
Attachment #8913122 -
Attachment is obsolete: true
Attachment #8913122 -
Flags: review?(mstange)
Updated•7 years ago
|
Attachment #8914652 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8914653 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8914654 -
Flags: review?(mstange) → review+
Comment 14•7 years ago
|
||
Comment on attachment 8914655 [details] [diff] [review]
(part 4) - Reduce tools/profiler/public/*.h to almost nothing in non-MOZ_GECKO_PROFILER builds
Review of attachment 8914655 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/base/nsFrameMessageManager.cpp
@@ +1537,5 @@
> bool aRunInGlobalScope)
> {
> + AUTO_PROFILER_LABEL_DYNAMIC(
> + "nsMessageManagerScriptExecutor::LoadScriptInternal", OTHER,
> + NS_LossyConvertUTF16toASCII(aURL).get());
This change is no good. The NS_LossyConvertUTF16toASCII temporary gets destroyed and we still hold on to the pointer from inside it for the rest of the function.
The same applies to some of the other places you're touching.
We could provide an alternative to AUTO_PROFILER_LABEL_DYNAMIC which accepts a const nsAString& and stores the NS_LossyConvertUTF16toASCII string in a member.
Attachment #8914655 -
Flags: review?(mstange) → review-
Assignee | ||
Comment 15•7 years ago
|
||
> > + AUTO_PROFILER_LABEL_DYNAMIC(
> > + "nsMessageManagerScriptExecutor::LoadScriptInternal", OTHER,
> > + NS_LossyConvertUTF16toASCII(aURL).get());
>
> This change is no good. The NS_LossyConvertUTF16toASCII temporary gets
> destroyed and we still hold on to the pointer from inside it for the rest of
> the function.
Ugh, yes, and I'm pretty sure you've stopped me from making the same mistake in the past.
Also, we have code like this:
> if (profiler_is_active()) {
> NS_LossyConvertUTF16toASCII messageNameCStr(aMessageName);
> AUTO_PROFILER_LABEL_DYNAMIC("nsFrameMessageManager::SendMessage", EVENTS,
> messageNameCStr.get());
> }
Here the scope for the string is valid, but it's also trivial -- we push and immediately pop the label. We can fix this by using Maybe<> to declare the string outside the scope and emplace() to create it within the scope.
I will fix these up.
Comment 16•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #15)
> Also, we have code like this:
>
> > if (profiler_is_active()) {
> > NS_LossyConvertUTF16toASCII messageNameCStr(aMessageName);
> > AUTO_PROFILER_LABEL_DYNAMIC("nsFrameMessageManager::SendMessage", EVENTS,
> > messageNameCStr.get());
> > }
>
> Here the scope for the string is valid, but it's also trivial -- we push and
> immediately pop the label.
Hahaha oops.
Assignee | ||
Comment 17•7 years ago
|
||
I've reverted the changes that made the string scopes incorrect. I will fix
the cases where the scopes are trivial in a follow-up.
Attachment #8914929 -
Flags: review?(mstange)
Attachment #8914929 -
Flags: feedback?(cpearce)
Assignee | ||
Updated•7 years ago
|
Attachment #8914655 -
Attachment is obsolete: true
Attachment #8914655 -
Flags: feedback?(cpearce)
Comment 18•7 years ago
|
||
Comment on attachment 8914929 [details] [diff] [review]
(part 4) - Reduce tools/profiler/public/*.h to almost nothing in non-MOZ_GECKO_PROFILER builds
Review of attachment 8914929 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/base/nsJSUtils.h
@@ +67,5 @@
>
>
> // ExecutionContext is used to switch compartment.
> class MOZ_STACK_CLASS ExecutionContext {
> +#if MOZ_GECKO_PROFILER
In some places you're using #if and in others #ifdef. Which one is correct?
Assignee | ||
Comment 19•7 years ago
|
||
> In some places you're using #if and in others #ifdef. Which one is correct?
$OBJDIR/mozilla-config.h contains this
> #define MOZ_GECKO_PROFILER 1
so either will work, but #ifdef should be used throughout for consistency. There are only 2 #if occurrences, I will fix them.
Reporter | ||
Comment 20•7 years ago
|
||
Comment on attachment 8914929 [details] [diff] [review]
(part 4) - Reduce tools/profiler/public/*.h to almost nothing in non-MOZ_GECKO_PROFILER builds
Thanks, I've tested this, and it works for us!
Attachment #8914929 -
Flags: feedback?(cpearce) → feedback+
Comment 21•7 years ago
|
||
Comment on attachment 8914929 [details] [diff] [review]
(part 4) - Reduce tools/profiler/public/*.h to almost nothing in non-MOZ_GECKO_PROFILER builds
Review of attachment 8914929 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good! GeckoProfiler.h is now even easier to follow. And the rule "Whenever you use something from the profiler that's not a (AUTO_)PROFILER_* macro, you need #ifdefs" should be easy to apply.
Attachment #8914929 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 22•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bafa74d94e2f8af7894f3139359e01e8985a74d
Bug 1403868 (part 1) - Remove argument from AutoProfilerInit(). r=mstange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b8e646e2e871bca8d30085b9f0221aa0f084b38
Bug 1403868 (part 2) - Tweak profiler_tracing(). r=mstange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/530bb3db13ad9a3fcb37d35dd5520e3e23b2e5cd
Bug 1403868 (part 3) - Rename the profiler's RAII macros. r=mstange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/672b02d8600717613ffe84ee46de8b277f1bc977
Bug 1403868 (part 4) - Reduce tools/profiler/public/*.h to almost nothing in non-MOZ_GECKO_PROFILER builds. r=mstange.
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3bafa74d94e2
https://hg.mozilla.org/mozilla-central/rev/4b8e646e2e87
https://hg.mozilla.org/mozilla-central/rev/530bb3db13ad
https://hg.mozilla.org/mozilla-central/rev/672b02d86007
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•