1280822 - Make gfxPrefs usable in the GPU process

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Since we don't initialize XPCOM in the GPU process, we can't use gfxPrefs. We'll have to synchronize gfxPrefs over IPDL instead, and make sure Preferences doesn't get used.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, don't use XPCOM

The first step is to make sure we can call gfxPrefs::GetSingleton() in the GPU process. There are two things preventing this:
 (1) after grabbing the default value, we then query Preferences for the live value
 (2) Moz2D has a "PreferenceAccess" abstraction that queries the live value of gfx.logging.level.

This patch fixes (1) by skipping calls. The patch to actually send the live value over IPDL will come later.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, change callbacks

gfxPrefs receives pref updates, but through the var cache, not a general callback. This adds a general callback mechanism.

In the parent process, all live gfxPrefs are watched so we can later synchronize them to the GPU process. In other processes we might still want to watch a preference, so there's an additional user callback mechanism for that.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, rm PreferenceAccess

I like the abstraction PreferenceAccess provides, but I can't see a way to easily implement it with the GPU process. It seems like we'd either need to build a totally new mechanism in parallel to gfxPrefs - or make an API that connects Moz2D directly to gfxPrefs instead of to Preferences.

Given that it's only used for one preference, I'm proposing we just remove the whole thing for now. gfxPrefs will instead manually hook sGfxLoggingLevel up to gfx.logging.level, and the code in this patch will work across all processes.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, send gfxPrefs over IPDL

This patch sends gfxPrefs over to the GPU process when it's first initialized, and again when a gfxPref's value is changed.

Because gfxPrefs hardcodes default values, we only need to send prefs that do not have their default value. Also, since the list of prefs in gfxPrefs is ordered, we can reference them by index instead of name. The IPDL traffic is pretty light as a result.

To implement this, prefs now add themselves to a static list, and cache their index into the list. The list's order is the same across processes.

When a new GPU process is initialized, any prefs with non-default values are sent to the GPU process. When a pref's value is changed in the UI process, its new value is sent to the GPU process as well. The flow for a pref change looks something like:
 1. UI Process: User changes about:config entry for gfx.blah.
 2. UI Process: gfxPref::Pref::OnChange fires for gfx.blah.
 3. UI Process: GPUChild::SendUpdatePref is called for gfx.blah.
 4. UI Process: gfxPref::Pref::FireChangeCallback fires.
 5. GPU Process: GPUParent::RecvUpdatePref is called for gfx.blah.
 6. GPU Process: gfxPref::Pref::FireChangeCallback fires for gfx.blah.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8763393 [details] [diff] [review]
part 1, don't use XPCOM

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

::: gfx/thebes/gfxPrefs.cpp
@@ +76,5 @@
> +// On lightweight processes such as for GMP and GPU, XPCOM is not initialized,
> +// and therefore we don't have access to Preferences. When XPCOM is not
> +// available we rely on manual synchronization of gfxPrefs values over IPC.
> +/* static */ bool
> +gfxPrefs::UsingXPCOM()

Feels like we should be able to ask Preferences whether it's available or not, rather than try to guess that based on the process type.

::: gfx/thebes/gfxPrefs.h
@@ +495,5 @@
>    static gfxPrefs* sInstance;
>    static bool sInstanceHasBeenDestroyed;
>  
>  private:
> +  static bool UsingXPCOM();

I'd rather see a method name that tells us why we want to know this, rather than how we may be checking (right now.)  For example "PreferencesSystemAvailable" or something like that.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8763395 [details] [diff] [review]
part 2, change callbacks

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

Reviewing in order, so it may be in the third patch, but I'm not sure right now why we need gfxPrefs::Prefs.  Makes some code complicated, introduces virtual function, etc.

::: gfx/thebes/gfxPrefs.cpp
@@ +85,5 @@
> +gfxPrefs::Pref::SetChangeCallback(ChangeCallback aCallback)
> +{
> +  mChangeCallback = aCallback;
> +
> +  if (!XRE_IsParentProcess() && UsingXPCOM()) {

If we make the IsParentProcess on gfxPrefs::Prefs instead of on gfxPrefs, we wouldn't need to mix the direct calls to IsParentProcess and XRE_IsParentProcess.

@@ +115,5 @@
>    }
>  }
>  
> +/* static */ bool
> +gfxPrefs::IsParentProcess()

This can probably be gfxPrefs::Prefs::IsParentProcess() and it could cache the value, so that we don't keep reevaluating XRE_IsParentProcess()?

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8763396 [details] [diff] [review]
part 3, rm PreferenceAccess

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

We will also want comments in Logging.h that the LOG_DEFAULT/LOG_CRITICAL/LOG_EVERYTHING values are being used in gfxPrefs, so any changes need to be propagated.

We probably don't need the original way of registering, where gfxPrefs doesn't need to be modified when we want to register a new one, and we lose the run time way of adding preferences, since we only have one example right now.  If that ever changes, we can revisit, but for now removing this functionality seems to be OK.

::: gfx/2d/Factory.cpp
@@ +152,5 @@
>  
>  namespace mozilla {
>  namespace gfx {
>  
> +int32_t LoggingPrefs::sGfxLogLevel = LOG_DEFAULT;

This needs comments that the preferences control this value.

::: gfx/thebes/gfxPrefs.h
@@ +154,5 @@
>      }
>      const char *Name() const override {
>        return Prefname();
>      }
> +    // When using XPCOM, the change callback can be triggered *before* our

This will benefit from a change from "are we using XPCOM" to "what kind of synchronization does the Preference system give us", even if it just a naming thing.

@@ +302,5 @@
>    DECL_GFX_PREF(Live, "gfx.layerscope.enabled",                LayerScopeEnabled, bool, false);
>    DECL_GFX_PREF(Live, "gfx.layerscope.port",                   LayerScopePort, int32_t, 23456);
>    // Note that        "gfx.logging.level" is defined in Logging.h
> +#if defined(DEBUG)
> +  DECL_GFX_PREF(Live, "gfx.logging.level",                     GfxLoggingLevel, int32_t, 5);

Don't like the magic numbers here.  That was the major reason that complication was introduced with the callbacks.

@@ +525,5 @@
>    {
>      MOZ_ASSERT(!sInstanceHasBeenDestroyed, "Should never recreate a gfxPrefs instance!");
>      if (!sInstance) {
>        sInstance = new gfxPrefs;
> +      sInstance->Init();

Doesn't feel like there is a value and a need for Init().  Given that the constructor does a lot of work already, may as well just put the additional stuff in the constructor and be done with it.  With Init(), we end up with asymmetrical "there is constructor + Init" vs. "there is destructor on its own".

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Milan Sreckovic [:milan] from comment #6)
> Comment on attachment 8763395 [details] [diff] [review]
> part 2, change callbacks
> 
> Review of attachment 8763395 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Reviewing in order, so it may be in the third patch, but I'm not sure right
> now why we need gfxPrefs::Prefs.  Makes some code complicated, introduces
> virtual function, etc.

Yes, it's needed in the 3rd and 4th patches. 

> If we make the IsParentProcess on gfxPrefs::Prefs instead of on gfxPrefs, we
> wouldn't need to mix the direct calls to IsParentProcess and
> XRE_IsParentProcess.

That was just a typo. No need to put it on gfxPrefs::Pref.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Milan Sreckovic [:milan] from comment #7)
> Comment on attachment 8763396 [details] [diff] [review]
> part 3, rm PreferenceAccess
> 
> Review of attachment 8763396 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +525,5 @@
> >    {
> >      MOZ_ASSERT(!sInstanceHasBeenDestroyed, "Should never recreate a gfxPrefs instance!");
> >      if (!sInstance) {
> >        sInstance = new gfxPrefs;
> > +      sInstance->Init();
> 
> Doesn't feel like there is a value and a need for Init().  Given that the
> constructor does a lot of work already, may as well just put the additional
> stuff in the constructor and be done with it.  With Init(), we end up with
> asymmetrical "there is constructor + Init" vs. "there is destructor on its
> own".

A separate initializer is needed because GetSingleton() reads sInstance, which isn't set at the time the constructor runs.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

It's hard to avoid magic values in gfxPrefs.h because it has to avoid including lots of other files. Maybe the trick is to keep PreferenceAccess, but give it a hardcoded method for each pref. I'll try that.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to David Anderson [:dvander] from comment #10)
> It's hard to avoid magic values in gfxPrefs.h because it has to avoid
> including lots of other files. Maybe the trick is to keep PreferenceAccess,
> but give it a hardcoded method for each pref. I'll try that.

I'd say the magic values are fine in this case, just add a lot of comments.  Everything else is going to be overly complicated.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, check Preferences

Check via Preferences instead of the process type.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, change callbacks

w/ typo fixed.

It's quite hard to implement parts 3 & 4 without having a base class for PrefTemplates, since we need something to give the non-templated methods in gfxPrefs.cpp. The virtual methods are only used for rare events, like synchronizing values.

re: caching the value of IsParentProcess - I don't think it'd buy us much, IsParentProcess is just returning a global variable already (mozilla::startup::sChildProcessType).

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, rm PreferenceAccess

It does feel pretty gross to hardcode these especially since the value depends on build flags. How about if we split the constants into a minimal header gfxPrefs.h can use?

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8763404 [details] [diff] [review]
part 4, send gfxPrefs over IPDL

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

::: gfx/ipc/GPUChild.cpp
@@ +36,5 @@
> +
> +    GfxPrefValue value;
> +    aPref->GetCachedValue(&value);
> +    prefs.AppendElement(GfxPrefSetting(aPref->Index(), value));
> +  });

Any reason you chose ForEachPref over instead having something like

for (gfxPrefs::Pref* pref : gfxPrefs::all()) {
}

?

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #15)
> Comment on attachment 8763404 [details] [diff] [review]
> part 4, send gfxPrefs over IPDL
> 
> Review of attachment 8763404 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/ipc/GPUChild.cpp
> @@ +36,5 @@
> > +
> > +    GfxPrefValue value;
> > +    aPref->GetCachedValue(&value);
> > +    prefs.AppendElement(GfxPrefSetting(aPref->Index(), value));
> > +  });
> 
> Any reason you chose ForEachPref over instead having something like
> 
> for (gfxPrefs::Pref* pref : gfxPrefs::all()) {
> }
> 
> ?

I was trying to avoid including nsTArray in gfxPrefs.h, but I can do that if you think it's cleaner.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

(In reply to David Anderson [:dvander] from comment #16)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #15)
> > Comment on attachment 8763404 [details] [diff] [review]
> > part 4, send gfxPrefs over IPDL
> > 
> > Review of attachment 8763404 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/ipc/GPUChild.cpp
> > @@ +36,5 @@
> > > +
> > > +    GfxPrefValue value;
> > > +    aPref->GetCachedValue(&value);
> > > +    prefs.AppendElement(GfxPrefSetting(aPref->Index(), value));
> > > +  });
> > 
> > Any reason you chose ForEachPref over instead having something like
> > 
> > for (gfxPrefs::Pref* pref : gfxPrefs::all()) {
> > }
> > 
> > ?
> 
> I was trying to avoid including nsTArray in gfxPrefs.h, but I can do that if
> you think it's cleaner.

Can you just export a simple iterator instead of nsTArray? I also don't really have a problem with nsTArray leaking into gfxPrefs.h it's probably most places already anyways.

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8763699 [details] [diff] [review]
part 1, check Preferences

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

Nice.

::: gfx/thebes/gfxPrefs.cpp
@@ +7,5 @@
>  
>  #include "mozilla/Preferences.h"
>  #include "MainThreadUtils.h"
>  #include "mozilla/gfx/Preferences.h"
> +#include "nsXULAppAPI.h"

We only need this include for the MOZ_ASSERT_IF below, right?  Nice.

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8763704 [details] [diff] [review]
part 2, change callbacks

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

If there are any answers to my questions that are "tricky", I'd put them as comments in the code, rather than (just) in the bug.

::: gfx/thebes/gfxPrefs.cpp
@@ +92,5 @@
> +gfxPrefs::Pref::SetChangeCallback(ChangeCallback aCallback)
> +{
> +  mChangeCallback = aCallback;
> +
> +  if (!IsParentProcess() && IsPrefsServiceAvailable()) {

We now have parent, (multiple) content, and GPU process - is the behavior of gfxPrefs split into two or three categories?  In other words, is this true only for content processes, or for content and parent process?

@@ +119,5 @@
>  
> +/* static */ bool
> +gfxPrefs::IsParentProcess()
> +{
> +  return XRE_IsParentProcess();

Is it worth caching this result?  As in:

static bool result = XRE_IsParentProcess();
return result;

I don't know how fast/expensive XRE_IsParentProcess() is.

::: gfx/thebes/gfxPrefs.h
@@ +157,5 @@
>          default:
>            MOZ_CRASH("Incomplete switch");
>        }
>      }
> +    const char *Name() const override {

I don't recall if our coding standards want us to have the explicit "virtual", or if the "override" is enough of a hint.  Don't really care either way, I think it's as obvious what's going on without it.

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8763707 [details] [diff] [review]
part 3, rm PreferenceAccess

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

Nice.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

Shutdown crash with the first three patches:
Assertion failure: IsPrefsServiceAvailable(), at /Users/msreckovic/Repos/mozilla-central/gfx/thebes/gfxPrefs.cpp:204
#01: gfxPrefs::UnwatchChanges(char const*, gfxPrefs::Pref*)[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin15.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1fd5ff0]
#02: gfxPrefs::PrefTemplate<(gfxPrefs::UpdatePolicy)2, bool, &(gfxPrefs::GetWebGL2CompatModePrefDefault()), &(gfxPrefs::GetWebGL2CompatModePrefName())>::~PrefTemplate()[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin15.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1ffac78]
#03: gfxPrefs::PrefTemplate<(gfxPrefs::UpdatePolicy)2, bool, &(gfxPrefs::GetWebGL2CompatModePrefDefault()), &(gfxPrefs::GetWebGL2CompatModePrefName())>::~PrefTemplate()[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin15.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1fd8785]
#04: gfxPrefs::~gfxPrefs()[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin15.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1fd4bf3]
#05: gfxPrefs::~gfxPrefs()[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin15.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1fd38f5]
#06: gfxPrefs::DestroySingleton()[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin15.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1fd3854]
#07: gfxPlatform::Shutdown()[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin15.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1fc73ac]

Comment 22

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Milan Sreckovic [:milan] from comment #19)
> Comment on attachment 8763704 [details] [diff] [review]
> part 2, change callbacks
> 
> Review of attachment 8763704 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> We now have parent, (multiple) content, and GPU process - is the behavior of
> gfxPrefs split into two or three categories?  In other words, is this true
> only for content processes, or for content and parent process?

Yes - the Set calls will not work at all in the GPU process. That was already true for content processes but now they'll assert instead. I'll make a note of that.

> @@ +119,5 @@
> >  
> > +/* static */ bool
> > +gfxPrefs::IsParentProcess()
> > +{
> > +  return XRE_IsParentProcess();
> 
> Is it worth caching this result?  As in:
> 
> static bool result = XRE_IsParentProcess();
> return result;
> 
> I don't know how fast/expensive XRE_IsParentProcess() is.

It should be sufficiently fast, it just reads a global variable. It's effectively already cached.

> ::: gfx/thebes/gfxPrefs.h
> @@ +157,5 @@
> >          default:
> >            MOZ_CRASH("Incomplete switch");
> >        }
> >      }
> > +    const char *Name() const override {
> 
> I don't recall if our coding standards want us to have the explicit
> "virtual", or if the "override" is enough of a hint.  Don't really care
> either way, I think it's as obvious what's going on without it.

I'm not sure either but I prefer just having override, virtual also implies a derived class could override it. This class is basically final.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

(In reply to David Anderson [:dvander] from comment #22)
> > I don't recall if our coding standards want us to have the explicit
> > "virtual", or if the "override" is enough of a hint.  Don't really care
> > either way, I think it's as obvious what's going on without it.
> 
> I'm not sure either but I prefer just having override, virtual also implies
> a derived class could override it. This class is basically final.

The coding standards say to just use override, since it implies the virtual. "Method declarations must use at most one of the following keywords: virtual, override, or final." [1]

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods

Comment 24

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, send gfxPrefs over IPDL, v2

w/ simple iterator instead.

Comment 25

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Milan Sreckovic [:milan] from comment #21)
> Shutdown crash with the first three patches:
> Assertion failure: IsPrefsServiceAvailable(), at
> /Users/msreckovic/Repos/mozilla-central/gfx/thebes/gfxPrefs.cpp:204

Looks like this is a bogus assert, the prefs service goes away before gfxPrefs, so we can't assert that it's still available.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3db77f57cef4
Don't initialize gfxPrefs with libpref in the GPU process. (bug 1280822 part 1, r=milan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/91715d8e468f
Add change callbacks to gfxPrefs. (bug 1280822 part 2, r=milan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/93d483583ffa
Remove PreferenceAccess. (bug 1280822 part 3, r=milan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9a0567cd7c1
Synchronize gfxPrefs to the GPU process. (bug 1280822 part 4, r=jrmuizel)

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for leaks https://hg.mozilla.org/integration/mozilla-inbound/rev/c6bb3db5e1b8

Happening in multiple jobs, but here's an example: https://treeherder.mozilla.org/logviewer.html#?job_id=30625606&repo=mozilla-inbound

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec2097515dc
Don't initialize gfxPrefs with libpref in the GPU process. (bug 1280822 part 1, r=milan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d787bc79425f
Add change callbacks to gfxPrefs. (bug 1280822 part 2, r=milan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/53b78e203d0a
Remove PreferenceAccess. (bug 1280822 part 3, r=milan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb97f090f9d4
Synchronize gfxPrefs to the GPU process. (bug 1280822 part 4, r=jrmuizel)

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6ec2097515dc
https://hg.mozilla.org/mozilla-central/rev/d787bc79425f
https://hg.mozilla.org/mozilla-central/rev/53b78e203d0a
https://hg.mozilla.org/mozilla-central/rev/cb97f090f9d4