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

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

({regression})

Trunk
mozilla39
All
macOS
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed, relnote-firefox 39+)

Details

(Whiteboard: gfx-noted)

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

4 years ago
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
(Assignee)

Updated

4 years ago
Keywords: regressionwindow-wanted
(Assignee)

Comment 1

4 years ago
Builds before tiling are fast, builds after tiling are slow, blaming bug 982338.
Blocks: 982338
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 :)
(Assignee)

Comment 5

4 years ago
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
(Assignee)

Comment 6

4 years ago
Posted patch part 1: skip unaffected tiles (obsolete) — Splinter Review
This didn't make it any faster.
Attachment #8563147 - Flags: review?(jmuizelaar)
(Assignee)

Comment 7

4 years ago
Posted 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?
(Assignee)

Comment 9

4 years ago
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.
(Assignee)

Comment 10

4 years ago
This helps because the perf problem is that CG spends ages iterating over dashes that are outside the clip.
(Assignee)

Updated

4 years ago
Blocks: 1133272
(Assignee)

Comment 11

4 years ago
Posted 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)
(Assignee)

Comment 12

4 years ago
Attachment #8566844 - Flags: review?(jmuizelaar)
(Assignee)

Comment 13

4 years ago
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 #8566845 - Flags: review?(jmuizelaar)
Attachment #8563151 - Flags: review?(jmuizelaar)
(Assignee)

Comment 15

4 years ago
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+
(Assignee)

Updated

4 years ago
Attachment #8566844 - Attachment is obsolete: true
(Assignee)

Comment 18

4 years ago
(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.
(Assignee)

Comment 20

4 years ago
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
Last Resolved: 4 years ago
status-firefox39: --- → fixed
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: --- → ?
(Assignee)

Updated

4 years ago
Depends on: 1151145
relnote-firefox: ? → 39+
Depends on: 1198307
See Also: → bug 1342571
You need to log in before you can comment on or make changes to this bug.