GeckoProfiler.h included in xpcom/threads if MOZ_INTERNAL_API is defined

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpearce, Assigned: njn)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

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.
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.
Attachment #8913122 - Flags: review?(n.nethercote)
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.
(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)
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-
(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.
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)
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
We can use |this| instead, as is already done in AutoProfilerRegisterThread().
Attachment #8914652 - Flags: review?(mstange)
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)
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)
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.
Attachment #8914655 - Flags: feedback?(cpearce)
Attachment #8913122 - Attachment is obsolete: true
Attachment #8913122 - Flags: review?(mstange)
Attachment #8914652 - Flags: review?(mstange) → review+
Attachment #8914653 - Flags: review?(mstange) → review+
Attachment #8914654 - Flags: review?(mstange) → review+
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-
> > +  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.
(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.
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)
Attachment #8914655 - Attachment is obsolete: true
Attachment #8914655 - Flags: feedback?(cpearce)
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?
> 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.
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 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+
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.
Blocks: 1384423
Blocks: 1378312
You need to log in before you can comment on or make changes to this bug.