1059033 - TiledDrawTarget has a lot of overhead for some operations

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Calling the same function on all the tiles can end up costing us a lot especially when the operation has no effect on most of the tiles.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Cull DrawTargetTiled operations

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Avoid really slow path bounds checks

The previous patch adds a call to GetStrokedBounds which is painfully slow for enormous paths. One of our crashtests does this and times out.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Avoid save/restoring for already clipped out surfaces

Saving and restoring the quartz gstate isn't cheap, so we should avoid doing it if we're already entirely clipped out. This is really common with tiling.

Comment 4

[Jeff Muizelaar [:jrmuizel] (parental leave until April 22)]

Comment on attachment 8479524 [details] [diff] [review]
Part 2: Avoid really slow path bounds checks

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

::: gfx/2d/PathCG.cpp
@@ +309,5 @@
> +  Rect bounds = GetBounds(aTransform);
> +  if (bounds.width > 100000.f ||
> +      bounds.height > 100000.f) {
> +    return bounds;
> +  }

This isn't correct. The stroked bounds are larger than the bounds. Who's calling GetStrokedBounds? Should they just not?

If this needs to happen you can approximate the stroke bounds from the bounds by taking the stroke width into account. There's code to do this in multiple places. (svg and cairo)

Comment 5

[Jeff Muizelaar [:jrmuizel] (parental leave until April 22)]

Comment on attachment 8479526 [details] [diff] [review]
Part 3: Avoid save/restoring for already clipped out surfaces

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

Not a huge fan of this patch. I don't have any better ideas yet though.

Comment 6

[Jeff Muizelaar [:jrmuizel] (parental leave until April 22)]

Comment on attachment 8479526 [details] [diff] [review]
Part 3: Avoid save/restoring for already clipped out surfaces

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

I'd prefer calling CGContextGetClipBoundingBox() and early returning instead of tracking this manually. Also this seems like a problem that other backends will potentially have and should maybe solved at a higher level.

Comment 7

[Matt Woodrow (:mattwoodrow)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> 
> This isn't correct. The stroked bounds are larger than the bounds. Who's
> calling GetStrokedBounds? Should they just not?
> 
> If this needs to happen you can approximate the stroke bounds from the
> bounds by taking the stroke width into account. There's code to do this in
> multiple places. (svg and cairo)

The caller is the part 1 patch. I can look at implementing the latter.

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> Comment on attachment 8479526 [details] [diff] [review]
> Part 3: Avoid save/restoring for already clipped out surfaces
> 
> Review of attachment 8479526 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd prefer calling CGContextGetClipBoundingBox() and early returning instead
> of tracking this manually. Also this seems like a problem that other
> backends will potentially have and should maybe solved at a higher level.

This won't solve the problem. The performance issue is calling CGContextSaveGState/CGContextRestoreGState from PushClipRect/PopClip.

We hit this when clipping before pushing a group, and we have to clip all the tiles individually.

I might be able to solve this in a shared way within gfxContext, I'll look into that.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Avoid save/restoring for already clipped out surfaces v2

Here's an alternative version of part 3, since Jeff doesn't like the other one.

Pushing and popping clips to all the tiles can be pretty expensive on some backends (quartz at least), so this avoids it.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Avoid recreating DrawTargets

The cost of this adds up when we have a lot of tiles. We already assert that we only have one reference, so this should be safe.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Avoid really slow path bounds checks v2

Comment 12

[Nicolas Silva [:nical]]

Comment on attachment 8480304 [details] [diff] [review]
Part 4: Avoid recreating DrawTargets

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

Shouldn't we also reset clips? Or do we know for sure that our drawing code pops all of them (in which case it'd be nice to have an assertion somewhere to ensure it stays true)?

Comment 13

[Jeff Muizelaar [:jrmuizel] (parental leave until April 22)]

Comment on attachment 8480309 [details] [diff] [review]
Part 2: Avoid really slow path bounds checks v2

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

Shouldn't the caller just do this?

Comment 14

[Matt Woodrow (:mattwoodrow)]

(In reply to Nicolas Silva [:nical] from comment #12)
> Comment on attachment 8480304 [details] [diff] [review]
> Part 4: Avoid recreating DrawTargets
> 
> Review of attachment 8480304 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Shouldn't we also reset clips? Or do we know for sure that our drawing code
> pops all of them (in which case it'd be nice to have an assertion somewhere
> to ensure it stays true)?

We don't have an API to reset clips or check that they've been reset, so we just have to trust it I guess?

We should be wrapping the DT in gfxContext that will undo any clips changes when its destructed.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Avoid really slow path bounds checks v3

Comment 16

[Bas Schouten (:bas.schouten)]

Comment on attachment 8480300 [details] [diff] [review]
Part 3: Avoid save/restoring for already clipped out surfaces v2

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

Looks good.

::: gfx/2d/DrawTargetTiled.cpp
@@ +111,5 @@
>    void \
>    DrawTargetTiled::command(type1 arg1) \
>    { \
>      for (size_t i = 0; i < mTiles.size(); i++) { \
> +      if (!mTiles[i].mClippedOut) \

nit: Technically we should add { }, but since we're inside a macro I don't care too much.

Comment 17

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Implement OptimizeSourceSurface for quartz

CGImage caches copies of scaled images internally, we want to take advantage of this when we're using the same image for multiple tiles.

Comment 18

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Clip text drawing

Calling FillGlyphs on every tile ends up adding a lot of overhead. It's not easy to cull FillGlyphs calls, so instead this clips to the text boundaries so that unaffected tiles are clipped out.

http://compare-talos.mattn.ca/?oldRevs=d697d649c765,8da1906596d6,1138e8f07c48,47c9418fbc28,2a15dc07ddaa&newRev=c35a86bbdf70&server=graphs.mozilla.org&submit=true

Compare talos looks pretty good for 10.8, only tsvg_opacity with a slight regression. We're also winning by a lot for tscrollx, but the test that improves isn't included in graphserver results.

Comment 19

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7eaac164c36
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ef5a3a9bb06
https://hg.mozilla.org/integration/mozilla-inbound/rev/301fdfea8fbf
https://hg.mozilla.org/integration/mozilla-inbound/rev/e59430ea4256
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e4105ea0a73
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc8dc49bba25

Comment 20

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8d79e4fbfc4

Comment 21

[Matt Woodrow (:mattwoodrow)]

Backed out part 6 for causing reftest failures. This might mean performance regresses a bit, I'll look into fixing it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/22f161c9ba3e

Comment 22

[Matt Woodrow (:mattwoodrow)]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/a54dbdca597b

Comment 23

[Phil Ringnalda (:philor)]

Backed out the lot of it in https://hg.mozilla.org/integration/mozilla-inbound/rev/6cddb4cf40e3 for still being busted on Windows, and possibly for https://tbpl.mozilla.org/php/getParsedLog.php?id=47152301&tree=Mozilla-Inbound after the backout of part 6, not clear if that's where that bustage came from.

Comment 24

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5bbf22f2f28
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e81f6f74182
https://hg.mozilla.org/integration/mozilla-inbound/rev/31e89a2a409f
https://hg.mozilla.org/integration/mozilla-inbound/rev/76897f52ac2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb3885b57d48
https://hg.mozilla.org/integration/mozilla-inbound/rev/07b3695aa02f

Comment 25

[Phil Ringnalda (:philor)]

And out again in https://hg.mozilla.org/integration/mozilla-inbound/rev/21b1152c66cc for build bustage.

Comment 26

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbffc438afd7
https://hg.mozilla.org/integration/mozilla-inbound/rev/dae2525a189c
https://hg.mozilla.org/integration/mozilla-inbound/rev/04f6cbbcd1bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/e96c73e49d58
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dbb91002381
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bea22f534f0

Comment 27

[Matt Woodrow (:mattwoodrow)]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/7729eecf463f

Comment 28

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aaa0c099b8ac

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cbffc438afd7
https://hg.mozilla.org/mozilla-central/rev/dae2525a189c
https://hg.mozilla.org/mozilla-central/rev/04f6cbbcd1bc
https://hg.mozilla.org/mozilla-central/rev/e96c73e49d58
https://hg.mozilla.org/mozilla-central/rev/7dbb91002381
https://hg.mozilla.org/mozilla-central/rev/2bea22f534f0
https://hg.mozilla.org/mozilla-central/rev/7729eecf463f
https://hg.mozilla.org/mozilla-central/rev/aaa0c099b8ac