Closed
Bug 1274624
Opened 6 years ago
Closed 6 years ago
Poor performance dragging element
Categories
(Core :: Graphics, defect)
Tracking
()
VERIFIED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | verified |
People
(Reporter: gcowie, Assigned: eflores)
References
Details
(Keywords: perf, Whiteboard: [gfx-noted])
Attachments
(2 files, 2 obsolete files)
8.55 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
3.37 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0 Build ID: 20160520030251 Steps to reproduce: http://pebblecode.com/404/ Click and drag the windows Actual results: The windows move very slowly, less than 1FPS I did a performance trace and the log shows 3+ second "Paint" events Expected results: Windows should move smoothly, 60FPS - as in Chrome and Internet Explorer
Comment 1•6 years ago
|
||
Reproducible. The windows move very slowly in Firefox as compared to other browsers. --- Version 49.0a1 Build ID 20160524030227 Update Channel nightly User Agent Mozilla/5.0 (Windows NT 6.3; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0
Status: UNCONFIRMED → NEW
Component: Untriaged → Graphics
Ever confirmed: true
Product: Firefox → Core
Assignee | ||
Comment 2•6 years ago
|
||
The slowness here is due to these CSS rules, used to draw the background: > background-image: linear-gradient(0deg, transparent 50%, #8592e1 50%),linear-gradient(90deg, #8592e1 50%, #000 50%); > background-size: 2px 2px, 2px 2px; The page is drawing the background using 2x2 pixel tiles, and we appear to be rendering the gradient for each one of these tiles [1]. This takes some time. [1] https://dxr.mozilla.org/mozilla-central/rev/3c5025f98e561a20e24d97c91a9e4e0ec28015ea/layout/base/nsCSSRendering.cpp#2836-2912
Comment 3•6 years ago
|
||
Commits pushed to master at https://github.com/taskcluster/taskcluster-tools https://github.com/taskcluster/taskcluster-tools/commit/3f6301bf11a9c930630c29a9dad9ccd3e81cc4ef Bug 1274624 - Fixes floating zero in taskview UI https://github.com/taskcluster/taskcluster-tools/commit/2fbd7391422bb93bf165298c5094cb5388b7b290 Merge pull request #117 from eliperelman/bug-1284624 Bug 1274624 - Fixes floating zero in taskview UI
Assignee | ||
Comment 4•6 years ago
|
||
This patch adds the ability for PaintGradient to paint to a temporary surface, and use that as a pattern to fill the provided rendering context. Since we snap each tile and aRepeatSize is rational in device space, we can't just tile by aRepeatSize. But we can determine a 'real' repeat size that takes snapping into account. We then paint to a surface of that size, and then paint from that surface to the original context. Not sure I like this patch, so just asking f? for now. A different approach, if we don't mind where we're rounding, would be to paint four tiles of size {floor,ceil}(aRepeatSize.width) x {floor,ceil}(aRepeatSize.height) and group all the roundings together. i.e. all tiles of floor(width) would be grouped to the left, and all tiles of ceil(width) would be grouped to the right; and similar vertically. We would still need our current code, though, in case we get a ridiculously large aRepeatSize.
Attachment #8770991 -
Flags: feedback?(mstange)
Comment 5•6 years ago
|
||
Is aRepeatSize fractional on this page? In other words, if you bail out for non-device pixel aligned repeat sizes, does it fix the problem on this page? I thought we had code that basically turned linear-gradient + background-repeat into a repeating-linear-gradient. Then we'd fill the whole area using one gradient fill. Is that code not kicking in here? Or am I misremembering? The GCD idea is brilliant. I like it. And the tilePixels.value() < dirtyPixels.value() check is spot on. A few months ago Mason sped up repeated scaled raster image drawing by scaling up one instance of the image and then repeating the scaled up surface. We only did that when the repeat size was aligned with device pixels, and just fell back to the old code when it was not. It fixed most of our image-scaling related problems on macOS, probably because most images are CSS pixel aligned and scale factors on macOS are usually 1.0 or 2.0 because not many people zoom.
Assignee | ||
Comment 6•6 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #5) > Is aRepeatSize fractional on this page? In other words, if you bail out for > non-device pixel aligned repeat sizes, does it fix the problem on this page? Yes, my first version of this patch did that, but I got a bit carried away trying to generalise it. > I thought we had code that basically turned linear-gradient + > background-repeat into a repeating-linear-gradient. Then we'd fill the whole > area using one gradient fill. Is that code not kicking in here? Or am I > misremembering? That does seem to be the case. If the gradient is axis-aligned and the length of the destination rect along that axis, we should be doing that. I'll investigate.
Assignee | ||
Comment 7•6 years ago
|
||
Just got the chance to look at this again today. The fast path won't be hit currently for any gradient whose first stop isn't at position 0[1]. This is because we don't use the original gradient line: we normalise the stops we have to the range [0, 1] so the start point isn't where we think it is, and we end up failing the check at [2]. In this particular case, the it *also* fails the |stopDelta != 0| check above that because all of the stops are in the same place. [1] Technically not quite correct -- any gradient whose first stop is on the destination rect boundary and meets all the other conditions should hit this path. [2] https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/layout/base/nsCSSRendering.cpp#2752
Assignee | ||
Comment 8•6 years ago
|
||
This patch makes sure we hit the fast path in this case, and now in most cases involving axis-aligned linear gradients.
Attachment #8770991 -
Attachment is obsolete: true
Attachment #8770991 -
Flags: feedback?(mstange)
Attachment #8773850 -
Flags: review?(mstange)
Comment 9•6 years ago
|
||
Comment on attachment 8773850 [details] [diff] [review] 1274624-linear.patch Review of attachment 8773850 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Can you add a reftest for at least one of the hard InterpolateColorStop cases? ::: layout/base/nsCSSRendering.cpp @@ +2713,5 @@ > + // right way up. > + if (lineStart.x > lineEnd.x || lineStart.y > lineEnd.y) { > + gfxPoint tmp = lineStart; > + lineStart = lineEnd; > + lineEnd = tmp; std::swap please.
Attachment #8773850 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 10•6 years ago
|
||
The only non-trivial (to me) part to InterpolateColorStop was getting the alpha blending correct. This test pokes at that in a way that would fail quite clearly.
Attachment #8774820 -
Flags: review?(mstange)
Comment 11•6 years ago
|
||
Comment on attachment 8774820 [details] [diff] [review] 1274624-test.patch Thanks. I expect you'll probably need some fuzzy() annotations though, because gradient rendering is always a little noisy, even when you don't expect it to be. Could you also add a test that compares a repeated linear-gradient to a repeating-linear-gradient? Maybe one that hits the swap + scale(-1) case.
Attachment #8774820 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 12•6 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #11) > Thanks. I expect you'll probably need some fuzzy() annotations though, > because gradient rendering is always a little noisy, even when you don't > expect it to be. Yep, will add that after a try run since it's green locally. > Could you also add a test that compares a repeated linear-gradient to a > repeating-linear-gradient? Maybe one that hits the swap + scale(-1) case. Done.
Attachment #8774820 -
Attachment is obsolete: true
Attachment #8774919 -
Flags: review?(mstange)
Comment 13•6 years ago
|
||
Comment on attachment 8774919 [details] [diff] [review] 1274624-test.patch Review of attachment 8774919 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8774919 -
Flags: review?(mstange) → review+
Comment 14•6 years ago
|
||
Pushed by eflores@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/72ae67aeb498 Speed up tiled axis-aligned linear gradient painting - r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/0d58d0626631 Test that colour interpolation is correct for clamped linear gradients - r=mstange
Comment 15•6 years ago
|
||
Pushed by eflores@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ef7be15608 Fuzz linear-size-1a.html on Windows - r=bustage
Comment 16•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/72ae67aeb498 https://hg.mozilla.org/mozilla-central/rev/0d58d0626631 https://hg.mozilla.org/mozilla-central/rev/f7ef7be15608
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Updated•6 years ago
|
Flags: qe-verify+
Comment 17•6 years ago
|
||
I was able to reproduce the issue on Nightly 49.0a1 (2016-05-10) on Win 10 Pro x64. I have verified the fix on Beta 50.0b3 Build 1 using Win 10 Pro x64, Ubuntu 16.04 LTS x64 and Mac OS Yosemite 10.10.4
Comment 18•6 years ago
|
||
Thank you, Carmen!
You need to log in
before you can comment on or make changes to this bug.
Description
•