Closed Bug 996226 Opened 6 years ago Closed 6 years ago

FishIEtank benchmark is slow

Categories

(Core :: Graphics, defect, P1, major)

All
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla31
blocking-b2g 1.4+
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: mwu, Assigned: mwu)

References

(Depends on 1 open bug, )

Details

(Keywords: perf, regression, Whiteboard: [c=benchmark p= s= u=1.4] [CR 640377])

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #989375 +++

The FishIEtank benchmark uses a very large image to store all the fish frames, which causes performance problems on B2G since the canvas image cache isn't big enough to store the moz2d version of the surface. Additionally, we don't cache moz2d copies of surfaces after bug 962670, so a copy is made every time we want to draw a fish.
blocking-b2g: --- → 1.4?
Status: NEW → ASSIGNED
blocking-b2g: 1.4? → 1.4+
This regression shows up in a lot of places. I think we need to consider backing out the volatile image stuff until we have a proper fix.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> This regression shows up in a lot of places. I think we need to consider
> backing out the volatile image stuff until we have a proper fix.

B2G only, no?

Backing out all of volatile images seems like a bad idea though, considering how big the changes are. It can be disabled by in imgFrame.cpp with a one line change.
I'm seeing a bunch of time on OS X creating SourceSurfaces under GetSourceSurfaceForSurface that I'm attributing to this change. Can we disable it on trunk for now?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> I'm seeing a bunch of time on OS X creating SourceSurfaces under
> GetSourceSurfaceForSurface that I'm attributing to this change. Can we
> disable it on trunk for now?

It seems like GetSourceSurfaceForSurface will hit the slow path when generating image layers on OSX since the default backend is coregraphics, not cairo. I suspect that would explain what you're seeing.
We can fix this bug, but the patches won't be particularly trivial so I think we should just disable this where the gfxASurface -> SourceSurface conversion is really expensive and done a lot. This basically means everywhere but Android.
Attachment #8407884 - Flags: review?(jmuizelaar)
We can and should probably be smarter in GetSourceSurfaceForSurface. Either that, or stop using it, and create Moz2D surfaces directly in imgFrame.

The problem is that when creating a SourceSurfaceCairo, we just wrap it around the existing cairo_surface_t, which is almost free. When creating SourceSurfaceCG, we instead make a copy of the data (since the backend doesn't understand how to hold reference to cairo objects).

There should be a way to avoid this in most cases, since it hurts us for both memory usage and performance.

One easy option would be to take the CreateWrappingDataSurface() path in GetSourceSurfaceForSurface if we can do so without copying (GetAsImageSurface returned non-null). However we *wouldn't* want that behaviour for backends that upload to the GPU, since we'd rather do that upfront and avoid potentially doing it every draw.
Actually, if the image data is available, we should probably do CreateWrappingDataSurface(), and then run it through OptimizeSourceSurface. That would give us 0 copies for CG, but still let GPU backends do the upload.
Depends on: 997551
I filed bug 997551 for my idea. Marked as a blocker on the assumption that this issue is one of the reasons for this performance regression.
(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> We can and should probably be smarter in GetSourceSurfaceForSurface. Either
> that, or stop using it, and create Moz2D surfaces directly in imgFrame.
> 

I'm working on generating Moz2D surfaces directly. Unfortunately, that's a pretty big change which should probably ride the next train. Maybe your idea can speed things up enough that we keep volatile images on..

> The problem is that when creating a SourceSurfaceCairo, we just wrap it
> around the existing cairo_surface_t, which is almost free. When creating
> SourceSurfaceCG, we instead make a copy of the data (since the backend
> doesn't understand how to hold reference to cairo objects).
> 

Core Graphics and Skia can actually be configured to release resources on surface destruction. Not sure about D2D though - poked around the docs today and didn't find a way.
(In reply to Michael Wu [:mwu] from comment #9)
> 
> I'm working on generating Moz2D surfaces directly. Unfortunately, that's a
> pretty big change which should probably ride the next train. Maybe your idea
> can speed things up enough that we keep volatile images on..

Awesome, happy to hear that. I've pushed it to try, we'll see if that shows any changes.


> Core Graphics and Skia can actually be configured to release resources on
> surface destruction. Not sure about D2D though - poked around the docs today
> and didn't find a way.

Yes, that's true. D2D wouldn't need to, since it just uploads to the GPU and it wouldn't be dependent on the source data.

We don't have Moz2D API for this (create a backend specific surface wrapping existing pixels and the caller will make sure they stay alive) currently though.
Attachment #8407884 - Flags: review?(jmuizelaar) → review+
Ok, so disabling volatile buffer images on master doesn't actually make this benchmark faster on B2G on master/1.5. However, it does work on 1.4. So I'm going to land this so it can bake a little and then get uplifted to 1.4.
https://hg.mozilla.org/mozilla-central/rev/7c5a96fbdd9a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 1005062
You need to log in before you can comment on or make changes to this bug.