Closed
Bug 1059033
Opened 7 years ago
Closed 7 years ago
TiledDrawTarget has a lot of overhead for some operations
Categories
(Core :: Graphics, defect)
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)
4.55 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
9.79 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
1.41 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
2.54 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
1.13 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
1.66 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8479523 -
Flags: review?(bas)
Assignee | ||
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
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)
Updated•7 years ago
|
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-
Assignee | ||
Comment 7•7 years ago
|
||
(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.
Assignee | ||
Comment 8•7 years ago
|
||
(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.
Assignee | ||
Comment 9•7 years ago
|
||
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)
Assignee | ||
Comment 10•7 years ago
|
||
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)
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #8480309 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•7 years ago
|
Attachment #8479524 -
Attachment is obsolete: true
Comment 12•7 years ago
|
||
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-
Assignee | ||
Comment 14•7 years ago
|
||
(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.
Assignee | ||
Comment 15•7 years ago
|
||
Attachment #8480309 -
Attachment is obsolete: true
Attachment #8480924 -
Flags: review?(jmuizelaar)
Comment 16•7 years ago
|
||
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+
Assignee | ||
Updated•7 years ago
|
Attachment #8479526 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8480304 -
Flags: review?(nical.bugzilla) → review+
Assignee | ||
Comment 17•7 years ago
|
||
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)
Assignee | ||
Comment 18•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8481194 -
Flags: review?(bas) → review+
Updated•7 years ago
|
Attachment #8481191 -
Flags: review?(jmuizelaar) → review+
Updated•7 years ago
|
Attachment #8480924 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 19•7 years ago
|
||
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
Assignee | ||
Comment 20•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8d79e4fbfc4
Assignee | ||
Comment 21•7 years ago
|
||
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
Assignee | ||
Comment 22•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a54dbdca597b
Comment 23•7 years ago
|
||
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.
Assignee | ||
Comment 24•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Comment 25•7 years ago
|
||
And out again in https://hg.mozilla.org/integration/mozilla-inbound/rev/21b1152c66cc for build bustage.
Assignee | ||
Comment 26•7 years ago
|
||
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
Assignee | ||
Comment 27•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7729eecf463f
Assignee | ||
Comment 28•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaa0c099b8ac
Comment 29•7 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•