Closed Bug 1059033 Opened 5 years ago Closed 5 years ago

TiledDrawTarget has a lot of overhead for some operations

Categories

(Core :: Graphics, defect)

29 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

Attachments

(6 files, 3 obsolete files)

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.
The previous patch adds a call to GetStrokedBounds which is painfully slow for enormous paths. One of our crashtests does this and times out.
Attachment #8479524 - Flags: review?(jmuizelaar)
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.
Attachment #8479526 - Flags: review?(jmuizelaar)
Attachment #8479523 - Flags: review?(bas) → review+
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)
Attachment #8479524 - Flags: review?(jmuizelaar) → review-
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 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.
Attachment #8479526 - Flags: review?(jmuizelaar) → review-
(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.
(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.
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.
Attachment #8480300 - Flags: review?(bas)
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.
Attachment #8480304 - Flags: review?(nical.bugzilla)
Attachment #8480309 - Flags: review?(jmuizelaar)
Attachment #8479524 - Attachment is obsolete: true
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 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?
Attachment #8480309 - Flags: review?(jmuizelaar) → review-
(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.
Attachment #8480309 - Attachment is obsolete: true
Attachment #8480924 - Flags: review?(jmuizelaar)
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.
Attachment #8480300 - Flags: review?(bas) → review+
Attachment #8479526 - Attachment is obsolete: true
Attachment #8480304 - Flags: review?(nical.bugzilla) → review+
CGImage caches copies of scaled images internally, we want to take advantage of this when we're using the same image for multiple tiles.
Attachment #8481191 - Flags: review?(jmuizelaar)
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.
Attachment #8481194 - Flags: review?(bas)
Attachment #8481194 - Flags: review?(bas) → review+
Attachment #8481191 - Flags: review?(jmuizelaar) → review+
Attachment #8480924 - Flags: review?(jmuizelaar) → review+
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
Keywords: leave-open
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.
Keywords: leave-open
Depends on: 1139269
Depends on: 1224522
You need to log in before you can comment on or make changes to this bug.