Closed Bug 1288259 Opened 3 years ago Closed 3 years ago

Introduce cross-process gfx variable cache

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(6 files, 3 obsolete files)

Most of the what gfxPlatform communicates to the compositor is data that is in between a preference and a computed value. For example, every compositor needs the tile size and vsync rate. The basic compositor additionally needs the content backend type.

In all of these cases, the value is determined once in the UI process. It is then recomputed in the child process and we usually hope that it's the same. In some cases, like the tile size, it's forced to be the same via a special IPC message.

I'm proposing a general mechanism to handle stuff like this, called "gfxVars". It is very similar to gfxPrefs - the major difference is greater flexibility in what the data types can be (they can be any IPDL-safe data type). You also don't need a preference name. Finally, values can only be pushed from the UI process: it is expected that child processes do need to change or modify its values.
Attached patch sketch (obsolete) — Splinter Review
Usage would look like:

  gfxVars::TileSize()
  gfxVars::SetTileSize(IntSize(w, h));

PContentParent and PGPUChild would inherit from gfxVarReceiver to make sure changes get propagated.
Attachment #8773061 - Flags: feedback?(jmuizelaar)
Attached patch patch (obsolete) — Splinter Review
More complete patch
Attachment #8773061 - Attachment is obsolete: true
Attachment #8773061 - Flags: feedback?(jmuizelaar)
Attachment #8773185 - Flags: feedback?(jmuizelaar)
Comment on attachment 8773185 [details] [diff] [review]
patch

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

Seems like a good idea to me.
Attachment #8773185 - Flags: feedback?(jmuizelaar) → feedback+
This generates a "void read(T* out)" function for each type in an IPDL union. This helps eliminate an antipattern sometimes seen (eg in gfxPrefs), where a template of type T can't generically read a value of type T out of the union.
Attachment #8774462 - Flags: review?(wmccloskey)
Attached patch part 2, gfxVars (obsolete) — Splinter Review
This introduces gfxVars and hooks it up to PContent and PGPU, but does not declare any variables yet.
Attachment #8773185 - Attachment is obsolete: true
Attachment #8774463 - Flags: review?(jmuizelaar)
Comment on attachment 8774463 [details] [diff] [review]
part 2, gfxVars

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

::: gfx/config/gfxVars.cpp
@@ +59,5 @@
> +  }
> +}
> +
> +/* static */ void
> +gfxVars::FetchNonDefaultVars(nsTArray<GfxVarUpdate>& aOutValues)

Might as well just have this return a nsTArray instead of having an out param.
Attachment #8774463 - Flags: review?(jmuizelaar) → review+
This moves tile size from gfxPlatform to gfxVars.
Attachment #8774510 - Flags: review?(jmuizelaar)
This adds the content backend to gfxVars. I didn't fix callers to gfxPlatform since there are a lot - I'd prefer to audit that in a separate bug, if necessary. The gfxVars version will initially only be used in the GPU process.
Attachment #8774513 - Flags: review?(jmuizelaar)
This moves XRender status to gfxVars.
Attachment #8774515 - Flags: review?(jmuizelaar)
This adds a bool for whether e10s is enabled, since BrowserTabsRemoteAutostart isn't available in the GPU process.
Attachment #8774516 - Flags: review?(jmuizelaar)
Comment on attachment 8774510 [details] [diff] [review]
part 3, move tile size

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

This is a nice improvement.
Attachment #8774510 - Flags: review?(jmuizelaar) → review+
Attachment #8774515 - Flags: review?(jmuizelaar) → review+
Attachment #8774516 - Flags: review?(jmuizelaar) → review+
Attachment #8774513 - Flags: review?(jmuizelaar) → review+
It'd be nice if these patches landed a day (or more) apart.  At least 1 vs. 2 vs. the rest.
I'll land just the IPC + tile size changes first, then the rest, since the early ones are the most risky.
I forgot to actually do PContent support. This is the same as part2, except ContentParent now registers itself as a receiver once ContentChild requests the initial variable set (which happens when gfxVars::Init is called in the child process).

Requesting review again since that's a sizable change.
Attachment #8774463 - Attachment is obsolete: true
Attachment #8775429 - Flags: review?(wmccloskey)
Attachment #8774462 - Flags: review?(wmccloskey) → review+
Comment on attachment 8775429 [details] [diff] [review]
part 2, gfxVars w/ addendum

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

::: dom/ipc/ContentParent.cpp
@@ +2605,5 @@
> +
> +void
> +ContentParent::OnVarChanged(const GfxVarUpdate& aVar)
> +{
> +  Unused << SendVarUpdate(aVar);

Can you check mIPCOpen here?

::: gfx/ipc/GPUChild.cpp
@@ +49,5 @@
> +
> +void
> +GPUChild::OnVarChanged(const GfxVarUpdate& aVar)
> +{
> +  SendUpdateVar(aVar);

Same thing here?
Attachment #8775429 - Flags: review?(wmccloskey) → review+
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8258601086d1
Generate template-friendly value readers for IPDL unions. (bug 1288259 part 1, r?=billm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9efbfe75ffe6
Introduce gfxVars for sharing graphics variables across processes. (bug 1288259 part 2, r=jrmuizel,billm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd55bf627226
Move TileSize from gfxPlatform to gfxVars. (bug 1288259 part 3, r=jrmuizel)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1d2b650e9e2
Add the 2D content backend to gfxVars. (bug 1288259 part 4, r=jrmuizel)
https://hg.mozilla.org/integration/mozilla-inbound/rev/39b14e19b257
Move UseXRender from gfxPlatformGtk to gfxVars. (bug 1288259 part 5, r=jrmuizel)
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb6e24811ad2
Add BrowserTabsRemoteAutostart to gfxVars. (bug 1288259 part 6, r=jrmuizel)
You need to log in before you can comment on or make changes to this bug.