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

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement, P3)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1403868 - Only include GeckoProfiler.h in xpcom/threads ifdef MOZ_GECKO_PROFILER.

The header should be included and used only if MOZ_GECKO_PROFILER is defined.

The gecko-media crate has to define MOZ_INTERNAL_API in order to make strings
work, and we'd like to avoid the Gecko profiler, so we'd like to switch the
profiler to being included based on its existing define rather than re-using
MOZ_INTERAL_API.

Review commit: https://reviewboard.mozilla.org/r/184542/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184542/

Comment 2

[Markus Stange [:mstange]]

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.

Comment 3

[Markus Stange [:mstange]]

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.

Comment 4

[Nicholas Nethercote [inactive]]

(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?

Comment 5

[Nicholas Nethercote [inactive]]

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

Comment 6

[Nathan Froyd [:froydnj]]

(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.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Include graph of GeckoProfiler.h

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.

Comment 8

[Nicholas Nethercote [inactive]]

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.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove argument from AutoProfilerInit()

We can use |this| instead, as is already done in AutoProfilerRegisterThread().

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Tweak profiler_tracing()

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.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Rename the profiler's RAII macros

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Reduce tools/profiler/public/*.h to almost nothing in non-MOZ_GECKO_PROFILER builds

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.

Comment 13

[Nicholas Nethercote [inactive]]

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.

Comment 14

[Markus Stange [:mstange]]

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.

Comment 15

[Nicholas Nethercote [inactive]]

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

[Markus Stange [:mstange]]

(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.

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Reduce tools/profiler/public/*.h to almost nothing in non-MOZ_GECKO_PROFILER builds

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.

Comment 18

[Markus Stange [:mstange]]

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?

Comment 19

[Nicholas Nethercote [inactive]]

> 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 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

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!

Comment 21

[Markus Stange [:mstange]]

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.

Comment 22

[Nicholas Nethercote [inactive]]

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

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

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