1249813 - nsShmImage doesn't reuse its image when different regions are passed to it

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Lee Salzman [:lsalzman]]

If the invalid region passed to nsShmImage::EnsureShmImage has a different size, the underlying XShm image will try to recreate itself rather than see if the new size fits within the old size. This bug was introduced by bug 1205045 because we had to avoid making GTK calls to query the window size directly.

Comment 1

[Lee Salzman [:lsalzman]]

Attachment: reuse nsShmImage if the new size fits within the old size

It should have been doing this to begin with, but since it used to rely on window size being rather fixed, it wasn't a problem. Now since we're using invalid regions enlarged to contain the origin, we need to be careful to reuse like this.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=35c3dc037d70

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: revise nsShmImage to allow draw targets anywhere inside its bounds

Slightly better approach, but more or less same principle. This cleans up the interface to nsShmImage to no longer use EnsureShmImage. You pass in the display/window/visual to the nsShmImage constructor, then explicitly call nsShmImage::CreateDrawTarget(bounds) and it handles all the reallocation automagically for you under the hood. A lot less messy...

This also fixes some performance regressions I accidentally introduced in bug 1019856 related to unnecessarily clearing the DT and properly constraining its size.

Comment 3

[Lee Salzman [:lsalzman]]

(In reply to Lee Salzman [:lsalzman] from comment #2)
> Created attachment 8721605 [details] [diff] [review]
> revise nsShmImage to allow draw targets anywhere inside its bounds
> 
> Slightly better approach, but more or less same principle. This cleans up
> the interface to nsShmImage to no longer use EnsureShmImage. You pass in the
> display/window/visual to the nsShmImage constructor, then explicitly call
> nsShmImage::CreateDrawTarget(bounds) and it handles all the reallocation
> automagically for you under the hood. A lot less messy...
> 
> This also fixes some performance regressions I accidentally introduced in
> bug 1019856 related to unnecessarily clearing the DT and properly
> constraining its size.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=45614d8af6ba

Comment 4

[Lee Salzman [:lsalzman]]

Attachment: revise nsShmImage to allow draw targets anywhere inside its bounds

One more round: this fixes some leftover issues in the non-OMTC path and makes the interface to nsShmImage::CreateDrawTarget and nsShmImage::Put more consistent.

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: make Cairo mark a surface as clear if clip covers entire surface

We aren't triggering Cairo's surface clearing optimization due to the following: https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetCairo.cpp?from=InitAlreadyReferenced#1778

Cairo was only checking if there was no clip path specified when a clear paint was done. But in this case, and sometimes we even explicitly push a clip to the DT, before doing a ClearRect, so Cairo never realizes the surface is clear.

With this small change, Cairo will now recognize the clip covers the entire surface while the clear is done, and thus mark the surface as clear appropriately.

Comment 6

[Lee Salzman [:lsalzman]]

Attachment: tell the compositor if the root layer has opaque content so it can skip clears

Final piece of this puzzle: in cases where the root layer is opaque, we are needlessly clearing the image surface representing the window, even though it will be overwritten. This shows up as up to 10% in the basic compositor profile, and almost all the time the root layer is opaque, so not clearing when we don't need to is an easy win.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8721605 [details] [diff] [review]
revise nsShmImage to allow draw targets anywhere inside its bounds

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

Shouldn't we just have a nsShmImage that's the size of the Window and not have to worry about this bounds business?

::: widget/nsShmImage.cpp
@@ +63,5 @@
>    mInfo.shmaddr = (char *)shmat(mInfo.shmid, nullptr, 0);
> +
> +  // Mark the handle removed so that it will destroy the segment when unmapped.
> +  shmctl(mInfo.shmid, IPC_RMID, 0);
> +

Why did you move this?

Comment 8

[Lee Salzman [:lsalzman]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> Comment on attachment 8721605 [details] [diff] [review]
> revise nsShmImage to allow draw targets anywhere inside its bounds
> 
> Review of attachment 8721605 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Shouldn't we just have a nsShmImage that's the size of the Window and not
> have to worry about this bounds business?
> 
> ::: widget/nsShmImage.cpp
> @@ +63,5 @@
> >    mInfo.shmaddr = (char *)shmat(mInfo.shmid, nullptr, 0);
> > +
> > +  // Mark the handle removed so that it will destroy the segment when unmapped.
> > +  shmctl(mInfo.shmid, IPC_RMID, 0);
> > +
> 
> Why did you move this?

In the failure case we were leaking the shm handles and never freeing them in the failure. Moving it to after the shmat means that if the shmat fails, there are on mappings, so the remove will happen immediately.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to Lee Salzman [:lsalzman] from comment #5)
> Created attachment 8722020 [details] [diff] [review]
> With this small change, Cairo will now recognize the clip covers the entire
> surface while the clear is done, and thus mark the surface as clear
> appropriately.

What clears does this defer?

Comment 10

[Lee Salzman [:lsalzman]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> (In reply to Lee Salzman [:lsalzman] from comment #5)
> > Created attachment 8722020 [details] [diff] [review]
> > With this small change, Cairo will now recognize the clip covers the entire
> > surface while the clear is done, and thus mark the surface as clear
> > appropriately.
> 
> What clears does this defer?

Mainly prevents double-clearing that may happen from places higher up in layers or layout trying to clear/render transparent background, since Cairo sees the surface is already clear and so skips it. Also it can speed up OP_SOURCE handling when Cairo knows the destination was initialized to clear.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Shouldn't we just have a nsShmImage that's the size of the Window and not have to worry about this bounds business?

Comment 12

[Lee Salzman [:lsalzman]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)
> Shouldn't we just have a nsShmImage that's the size of the Window and not
> have to worry about this bounds business?

That was fallout from bug 1205045. We unfortunately can't call into GTK to get the size, since that's not thread safe.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8722019 [details] [diff] [review]
revise nsShmImage to allow draw targets anywhere inside its bounds

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

It might be worth adding a comment that contains something about: "That was fallout from bug 1205045. We unfortunately can't call into GTK to get the size, since that's not thread safe."

Comment 14

[Lee Salzman [:lsalzman]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #13)
> Comment on attachment 8722019 [details] [diff] [review]
> revise nsShmImage to allow draw targets anywhere inside its bounds
> 
> Review of attachment 8722019 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It might be worth adding a comment that contains something about: "That was
> fallout from bug 1205045. We unfortunately can't call into GTK to get the
> size, since that's not thread safe."

Okay, will do.

Comment 15

[Lee Salzman [:lsalzman]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6628713807e1

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a90066c54f7c
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c4e6d47312a
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd59893f50b8

Comment 17

[William Lachance (:wlach)]

A bunch of performance changes from the above, mostly improvements though the following tests got worse:

* tscrollx non-e10s (e10s configuration improved)
* tp5o_scroll non-e10s
* glterrain e10s

Comment 18

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #17)
> A bunch of performance changes from the above, mostly improvements though
> the following tests got worse:
> 
> * tscrollx non-e10s (e10s configuration improved)
> * tp5o_scroll non-e10s
> * glterrain e10s

Link: https://treeherder.allizom.org/perf.html#/alerts?id=285

Comment 19

[Lee Salzman [:lsalzman]]

(In reply to William Lachance (:wlach) from comment #18)
> (In reply to William Lachance (:wlach) from comment #17)
> > A bunch of performance changes from the above, mostly improvements though
> > the following tests got worse:
> > 
> > * tscrollx non-e10s (e10s configuration improved)
> > * tp5o_scroll non-e10s
> > * glterrain e10s
> 
> Link: https://treeherder.allizom.org/perf.html#/alerts?id=285

Yep, I am aware of the results. There is something weird about try in the non-e10s case that causes tscrollx and similar benchmarks to be antagonized, whereas that pattern of results doesn't show up in my local testing (all tests improved here). I spent a lot of time investigating this phenomenon in another bug and could not identify a cause. So long as e10s and my local non-e10s results were improved, I'm okay with such as it is - so WONTFIX.

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/a90066c54f7c
https://hg.mozilla.org/mozilla-central/rev/1c4e6d47312a
https://hg.mozilla.org/mozilla-central/rev/bd59893f50b8