Closed Bug 1249813 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Graphics, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: lsalzman, Assigned: lsalzman)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
OS: Unspecified → Linux
Hardware: Unspecified → All
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
Attachment #8721531 - Flags: review?(jmuizelaar)
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.
Attachment #8721531 - Attachment is obsolete: true
Attachment #8721531 - Flags: review?(jmuizelaar)
Attachment #8721605 - Flags: review?(jmuizelaar)
(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
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.
Attachment #8721605 - Attachment is obsolete: true
Attachment #8721605 - Flags: review?(jmuizelaar)
Attachment #8722019 - Flags: review?(jmuizelaar)
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.
Attachment #8722020 - Flags: review?(jmuizelaar)
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.
Attachment #8722021 - Flags: review?(matt.woodrow)
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 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.
Attachment #8722021 - Flags: review?(matt.woodrow) → review+
(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?
(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.
Attachment #8722020 - Flags: review?(jmuizelaar) → review+
Shouldn't we just have a nsShmImage that's the size of the Window and not have to worry about this bounds business?
Flags: needinfo?(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.
Flags: needinfo?(lsalzman)
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."
Attachment #8722019 - Flags: review?(jmuizelaar) → review+
(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.
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
(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
(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.
Depends on: 1262995
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: