Closed Bug 1123019 Opened 9 years ago Closed 9 years ago

Long dotted / dashed borders paint extremely slowly on Mac, even if only a screenful of them is visible

Categories

(Core :: Graphics, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed
relnote-firefox --- 39+

People

(Reporter: mstange, Assigned: mstange)

References

Details

(Keywords: regression, Whiteboard: gfx-noted)

Attachments

(4 files, 5 obsolete files)

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:38.0) Gecko/20100101 Firefox/38.0

I noticed this on a long reddit thread. It looks like CoreGraphics iterates over all line segments of the border and is very bad at ignoring those outside the visible bounds.

I think this is a regression. At least I don't remember it being this bad before. Tiling will have made this worse, but it might also be a regression in CG from 10.9 to 10.10.

Almost all the time is spent in this stack:

> nsCSSBorderRenderer::DrawDashedSide(mozilla::Side)
> mozilla::gfx::DrawTargetTiled::StrokeLine
> mozilla::gfx::DrawTargetCG::StrokeLine
> ripc_DrawPathCGContextDrawPath
> CGPathApplyRIPRenderPath
> CG::Path::Sequence::apply
> CG::Path::Subpath::apply
> CG::Chunk::apply
> path_iterator_add_line_to_point
Builds before tiling are fast, builds after tiling are slow, blaming bug 982338.
Blocks: osx-tiling
DrawTargetTiled attempts to avoid sending draw calls to tiles that won't be affected, but it doesn't yet do this for StrokeLine (or StrokeRect).

Implementing that should be easy and would fix the regression.
Markus, is this something you can take?  Based on the time honoured "you find it, you fix it" approach :)
Sure.

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> DrawTargetTiled attempts to avoid sending draw calls to tiles that won't be
> affected, but it doesn't yet do this for StrokeLine (or StrokeRect).
> 
> Implementing that should be easy and would fix the regression.

It feels like the slowdown factor here is higher than the number of tiles, though. But I'll try that anyway.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Attached patch part 1: skip unaffected tiles (obsolete) — Splinter Review
This didn't make it any faster.
Attachment #8563147 - Flags: review?(jmuizelaar)
Attached patch part 2: shorten lines (obsolete) — Splinter Review
This made all the difference. But it's a good chunk of code, probably way over-engineered, and needs lots of comments. Before I sit down and add those comments, do you think this is something we should do or do you have a different idea?
Attachment #8563151 - Flags: review?(jmuizelaar)
(In reply to Markus Stange [:mstange] from comment #7)
> Created attachment 8563151 [details] [diff] [review]
> part 2: shorten lines
> 
> This made all the difference. But it's a good chunk of code, probably way
> over-engineered, and needs lots of comments. Before I sit down and add those
> comments, do you think this is something we should do or do you have a
> different idea?

Can you summarize what it does?
It wraps a PathBuilder, and replaces LineTo calls with MoveTo, LineTo, MoveTo calls when a encountering a line that isn't completely inside the clip rect, so that only the visible part of the line (+ a little error margin) gets drawn, all the time ensuring that the dash pattern looks the same as if the line were drawn unshortened.
This helps because the perf problem is that CG spends ages iterating over dashes that are outside the clip.
Blocks: 1133272
Attached patch part 1: skip unaffected tiles (obsolete) — Splinter Review
Fixes <cmath> build failure and also implements the culling for StrokeLine.
Attachment #8563147 - Attachment is obsolete: true
Attachment #8563147 - Flags: review?(jmuizelaar)
Attachment #8566843 - Flags: review?(jmuizelaar)
Attachment #8566844 - Flags: review?(jmuizelaar)
The previous approach wasn't working because you can't close a path that has been chopped up, and you need it to work for the top left corner of stroked rectangles.
Attachment #8563151 - Attachment is obsolete: true
Attachment #8563151 - Flags: review?(jmuizelaar)
Attachment #8566845 - Flags: review?(jmuizelaar)
I hope I've really fixed the build failure this time.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eae58a04314
Attachment #8566843 - Attachment is obsolete: true
Attachment #8566843 - Flags: review?(jmuizelaar)
Attachment #8567196 - Flags: review?(jmuizelaar)
Comment on attachment 8566844 [details] [diff] [review]
part 2: factor out stroking in DrawTargetCG

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

Lets not. This adds some overhead that we're not particularly in need of.
Attachment #8566844 - Flags: review?(jmuizelaar) → review-
Comment on attachment 8566845 [details] [diff] [review]
part 3: shrink clipped stroked rectangles and lines in DrawTargetCG

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

::: gfx/2d/DrawTargetCG.cpp
@@ +998,5 @@
> +  for (size_t i = 0; i < aStrokeOptions.mDashLength; i++) {
> +    length += aStrokeOptions.mDashPattern[i];
> +  }
> +  if (aStrokeOptions.mDashLength == 1) {
> +    // 1-element dash arrays are interpreted as a symmetric 2-element dash array.

The should be all odd dash arrays. i.e.
if (aStrokeOptions.mDashLength & 1)

https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/stroke-dasharray

@@ +1053,5 @@
> +}
> +
> +// Liang-Barsky
> +// Sets aStart and aEnd to floats between 0 and the line length, or returns
> +// false if the line is completely outside the rect.

You should add a note as to why you chose Liang-Barsky over other options.

Does this include the improvements suggested here?
"Some Improvements to a Parametric Line Clipping Algorithm"
Attachment #8566845 - Flags: review?(jmuizelaar) → review+
Attachment #8567196 - Flags: review?(jmuizelaar) → review+
Attachment #8566844 - Attachment is obsolete: true
(In reply to Jeff Muizelaar [:jrmuizel] from comment #17)
> Comment on attachment 8566845 [details] [diff] [review]
> part 3: shrink clipped stroked rectangles and lines in DrawTargetCG
> 
> Review of attachment 8566845 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawTargetCG.cpp
> @@ +998,5 @@
> > +  for (size_t i = 0; i < aStrokeOptions.mDashLength; i++) {
> > +    length += aStrokeOptions.mDashPattern[i];
> > +  }
> > +  if (aStrokeOptions.mDashLength == 1) {
> > +    // 1-element dash arrays are interpreted as a symmetric 2-element dash array.
> 
> The should be all odd dash arrays. i.e.
> if (aStrokeOptions.mDashLength & 1)
> 
> https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/stroke-dasharray

Thanks, that makes a lot more sense.

> 
> @@ +1053,5 @@
> > +}
> > +
> > +// Liang-Barsky
> > +// Sets aStart and aEnd to floats between 0 and the line length, or returns
> > +// false if the line is completely outside the rect.
> 
> You should add a note as to why you chose Liang-Barsky over other options.

Added a comment:
// This algorithm was chosen for its code brevity, with the hope that its
// performance is good enough.

> Does this include the improvements suggested here?
> "Some Improvements to a Parametric Line Clipping Algorithm"

Doesn't look like it. I actually think the original code I was looking at had the improvement, and I removed it because I wanted to have extra ifs tainting the std::min/max lines.
I have this check at the very end:
  if (t0 > t1) {
    return false;
  }
The improvement suggested in that document is basically "move that check into the loop, so you can skip checking the remaining edges", if I understood correctly.
I have a hard time believing that at most three additional float divisions per line are going to hurt much.
Attachment #8566845 - Attachment is obsolete: true
(In reply to Markus Stange [:mstange] from comment #18)

> 
> > Does this include the improvements suggested here?
> > "Some Improvements to a Parametric Line Clipping Algorithm"
> 
> Doesn't look like it. I actually think the original code I was looking at
> had the improvement, and I removed it because I wanted to have extra ifs
> tainting the std::min/max lines.
> I have this check at the very end:
>   if (t0 > t1) {
>     return false;
>   }
> The improvement suggested in that document is basically "move that check
> into the loop, so you can skip checking the remaining edges", if I
> understood correctly.
> I have a hard time believing that at most three additional float divisions
> per line are going to hurt much.

Might as well add something about this to the comment too.
Actually, let me just move that if up into the loop body. That way, the number of lines of code stays the same, we get the behavior we want, and we don't need the comment.
https://hg.mozilla.org/mozilla-central/rev/4558317a4a9c
https://hg.mozilla.org/mozilla-central/rev/d472aa32cecf
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Attachment #8574048 - Flags: review?(jmuizelaar) → review+
Release Note Request (optional, but appreciated)
[Why is this notable]: Fixes a regression from Firefox 35.
[Suggested wording]: Performance of displaying dashed lines on OS X is improved.
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Depends on: 1151145
Depends on: 1198307
See Also: → 1342571
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: