Last Comment Bug 657141 - Page scroll is lagged and slow
: Page scroll is lagged and slow
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: 5 Branch
: x86 Windows 7
: -- normal with 2 votes (vote)
: mozilla12
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
http://piro.sakura.ne.jp/xul/xul.html.en
Depends on:
Blocks: 100951 600760
  Show dependency treegraph
 
Reported: 2011-05-14 09:18 PDT by Alice0775 White
Modified: 2013-12-27 14:36 PST (History)
23 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-


Attachments
Change popping to only pop to common ancestor (1.83 KB, patch)
2011-05-14 11:33 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review-
Details | Diff | Splinter Review
Change popping to only pop to common ancestor v2 (2.32 KB, patch)
2011-05-17 10:53 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
asa: approval‑mozilla‑beta-
Details | Diff | Splinter Review
Part 2: Only optimize away clip levels when using a solid alpha mask (1.01 KB, patch)
2011-05-17 19:23 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review

Description Alice0775 White 2011-05-14 09:18:32 PDT
Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/8404426ef391
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110514 Firefox/6.0a1 ID:20110514030626
and
http://hg.mozilla.org/releases/mozilla-aurora/rev/4187f7b5edaf
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.2) Gecko/20110508 Firefox/5.0a2 ID:20110514042002

Page scroll is  lagged and slow.

This not happen on
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.2pre) Gecko/20110512 Firefox/4.0.2pre ID:20110512030229

Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2. Open URL
3. Scroll by mouse wheel or by draging thumb of scroll bar


Actual Results:
 Page scroll is  lagged and slow.

Expected Results:
 Page scroll should not be lagged.

Regression window(cached m-c hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/832de5e41bd2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.2a1pre) Gecko/20110411 Firefox/4.2a1pre ID:20110411140448
Fails:
http://hg.mozilla.org/mozilla-central/rev/e29f195869ad
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.2a1pre) Gecko/20110411 Firefox/4.2a1pre ID:20110411165559
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=832de5e41bd2&tochange=e29f195869ad
Suspected bug:
e29f195869ad	Bas Schouten — Bug 600760: Optimize mask with rectangular clip. r=jrmuizel
Comment 1 Bas Schouten (:bas.schouten) 2011-05-14 10:43:21 PDT
So, the reason here is that for this page's structure we're creating excessive clip pops. If the clip path does not match the current path we completely pop the entire stack of paths, in this case we only need to pop one.

I can provide an easy patch for this that is pretty safe.

Alternatively we can back this out completely for Fx5, depending on the amount of risk we're willing to manage.
Comment 2 Bas Schouten (:bas.schouten) 2011-05-14 11:33:49 PDT
Created attachment 532454 [details] [diff] [review]
Change popping to only pop to common ancestor

This patch should fix the problem and be quite safe. So we'd need to decide what to do with it. Currently doing an optimized build to confirm the fix.
Comment 3 Jeff Muizelaar [:jrmuizel] 2011-05-16 08:52:23 PDT
My gut feeling right now is that we should back out on 5
Comment 4 Bas Schouten (:bas.schouten) 2011-05-16 08:59:28 PDT
I would agree, if I didn't think it was pretty important to get competitive benchmark scores with IE9. Something we don't get on a bunch of them (like SpeedReading) without this patch :s. I think it's safe enough to take into Beta and see if we run into any trouble there (it'll still be trivial to back out). It relies on already extensively used find_common_ancestor code and there's few things that could go wrong.
Comment 5 Jeff Muizelaar [:jrmuizel] 2011-05-16 15:34:19 PDT
Comment on attachment 532454 [details] [diff] [review]
Change popping to only pop to common ancestor

Shouldn't temporary_clip have smaller scope.
Comment 6 Bas Schouten (:bas.schouten) 2011-05-16 16:07:49 PDT
(In reply to comment #5)
> Comment on attachment 532454 [details] [diff] [review] [review]
> Change popping to only pop to common ancestor
> 
> Shouldn't temporary_clip have smaller scope.

I don't think it can be in a smaller scope can it? It needs to be around for as long as actual_clip is since actual_clip is just a pointer to it, otherwise it could become a dangling pointer to a place on the stack.
Comment 7 Jeff Muizelaar [:jrmuizel] 2011-05-16 16:54:16 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 532454 [details] [diff] [review] [review] [review]
> > Change popping to only pop to common ancestor
> > 
> > Shouldn't temporary_clip have smaller scope.
> 
> I don't think it can be in a smaller scope can it? It needs to be around for
> as long as actual_clip is since actual_clip is just a pointer to it,
> otherwise it could become a dangling pointer to a place on the stack.

Indeed.
Comment 8 Jeff Muizelaar [:jrmuizel] 2011-05-16 21:09:19 PDT
Comment on attachment 532454 [details] [diff] [review]
Change popping to only pop to common ancestor

> 
> 	    if (clip->path != d2dsurf->clip.path) {
>-		// Only reset the clip if we don't have the right clip set. Otherwise
>-		// just leave the clip as it is. If we have the right clip set we
>-		// should not do a needless pop of the clip.
>-		actual_clip = NULL;
>+		// If we have a clip set, but it's not the right one, pass in
>+		// a temporary clip for the common ancestor to avoid excessive
>+		// popping.

This comment doesn't make sense to me. Specifically "pass in a temporary clip for the common ancestor".
It's not clear where the temporary clip is being passed and what "for the common ancestor" means.

>+		temporary_clip.path = find_common_ancestor(clip->path, d2dsurf->clip.path);

Does things all work out if there is no common ancestor? i.e. previously actual_clip would be NULL now it's not.

>+		temporary_clip.all_clipped = 0;

It's not immediately obvious to me how we know that all_clipped is false. Also you should probably use
FALSE here.
Comment 9 Bas Schouten (:bas.schouten) 2011-05-16 21:46:08 PDT
(In reply to comment #8)
> Comment on attachment 532454 [details] [diff] [review] [review]
> Change popping to only pop to common ancestor
> 
> > 
> > 	    if (clip->path != d2dsurf->clip.path) {
> >-		// Only reset the clip if we don't have the right clip set. Otherwise
> >-		// just leave the clip as it is. If we have the right clip set we
> >-		// should not do a needless pop of the clip.
> >-		actual_clip = NULL;
> >+		// If we have a clip set, but it's not the right one, pass in
> >+		// a temporary clip for the common ancestor to avoid excessive
> >+		// popping.
> 
> This comment doesn't make sense to me. Specifically "pass in a temporary
> clip for the common ancestor".
> It's not clear where the temporary clip is being passed and what "for the
> common ancestor" means.

We pass it to _cairo_d2d_set_clip later on. For the common ancestor means the common ancestor of the clip path currently set and the clip path requested. This is the point we'd usually pop to in _cairo_d2d_set_clip (see cairo-d2d-surface.cpp:1030). But we'd then push additional clips which, if we hit this code are not needed. Therefor we pass in the common ancestor so _cairo_d2d_set_clip will just pop until it hits that, and not push the (not needed) clip(s) lower down in the chain.

> 
> >+		temporary_clip.path = find_common_ancestor(clip->path, d2dsurf->clip.path);
> 
> Does things all work out if there is no common ancestor? i.e. previously
> actual_clip would be NULL now it's not.

Yes, see cairo-d2d-surface.cpp:1010, and your own comment there, which I believe is right :).

> 
> >+		temporary_clip.all_clipped = 0;
> 
> It's not immediately obvious to me how we know that all_clipped is false.
> Also you should probably use
> FALSE here.

We don't use it down the line in our clip setting code, so it's not really relevant (see _cairo_d2d_set_clip). But if it is true I believe cairo will not pass the call to the backend in an earlier stage.
Comment 10 Bas Schouten (:bas.schouten) 2011-05-17 10:53:29 PDT
Created attachment 533010 [details] [diff] [review]
Change popping to only pop to common ancestor v2

Attempt at rephrasing/adding comments.
Comment 11 Bas Schouten (:bas.schouten) 2011-05-17 12:30:33 PDT
http://hg.mozilla.org/mozilla-central/rev/66e854c53a11
Comment 12 Asa Dotzler [:asa] 2011-05-17 15:03:57 PDT
Is this something you all would like to see us take into Beta for Firefox 5? If so, please nominate with an approval request and a risk analysis.
Comment 13 Bas Schouten (:bas.schouten) 2011-05-17 15:11:57 PDT
(In reply to comment #12)
> Is this something you all would like to see us take into Beta for Firefox 5?
> If so, please nominate with an approval request and a risk analysis.

How do I place such a nomination?

As for a risk analysis, we have three options.

A) Take the fix, this exercises code that has been in use for quite a while and doesn't do anything particularly exciting. I think the fix is very low risk.

B) A backout of the patch causing this regression in 5 mentioned in the bug is higher risk I believe since it makes us 2x slower on for example SpeedReading, making us no long be as fast as (or even slightly faster) IE9. Which would be quite a shame from a competitive point of view.

C) Leaving this regression in, it doesn't seem to affect too many websites badly but at the same time this is hard to estimate. It could cause an overall slower user experience not significant enough to be noticeable to the average user as 'a bug' but still people might be experiencing it particularly on slower machines.

Personally I'm in favor of taking this fix for beta. The risk is very small and beta seems to provide sufficient testing coverage.
Comment 14 Asa Dotzler [:asa] 2011-05-17 15:46:15 PDT
Joe got it -- an approval request on the patch.
Comment 15 Alice0775 White 2011-05-17 15:47:29 PDT
I tested URL with following latest m-c hourly.
However, It seems that nothing is improved. The problem still exist.

http://hg.mozilla.org/mozilla-central/rev/e816b1abd0e0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110516 Firefox/6.0a1 ID:20110517131852
Comment 16 Bas Schouten (:bas.schouten) 2011-05-17 16:04:17 PDT
I retested locally. The problem is definitely improved a lot for me. However I might still be seeing a tiny difference with Fx 4.0.1. However that might also be PGO, I'm not sure. In any case this patch does improve things a lot for me...
Comment 17 Alice0775 White 2011-05-17 16:34:52 PDT
Umm strange,
*I checked with HWA (default clean profile).

Works fine, no scroll lag (before landing Bug600760):
http://hg.mozilla.org/mozilla-central/rev/832de5e41bd2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.2a1pre) Gecko/20110411 Firefox/4.2a1pre ID:20110411140448

Scroll lag exist (after landing Bug600760):
http://hg.mozilla.org/mozilla-central/rev/e29f195869ad
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.2a1pre) Gecko/20110411 Firefox/4.2a1pre ID:20110411165559

Scroll lag exist and nothing improved for me (after landing this bug):
http://hg.mozilla.org/mozilla-central/rev/e816b1abd0e0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110516 Firefox/6.0a1 ID:20110517131852

*If I disabled HWA,all of them work fine.

*Graphics
  Adapter Description: ATI Radeon HD 4300/4500 Series
  Vendor ID: 1002
  Device ID: 954f
  Adapter RAM: 512
  Adapter Drivers: aticfx64 aticfx64 aticfx32 aticfx32 atiumd64 atidxx64 atiumdag atidxx32 atiumdva atiumd6a atitmm64
  Driver Version: 8.850.0.0
  Driver Date: 4-19-2011
  Direct2D Enabled: true
  DirectWrite Enabled: true (6.1.7601.17563)
  ClearType Parameters: Gamma: 2200 Pixel Structure: RGB ClearType Level: 50 Enhanced Contrast: 50 
  WebGL Renderer: Google Inc. -- ANGLE -- OpenGL ES 2.0 (ANGLE 0.0.0.611)
  GPU Accelerated Windows: 1/1 Direct3D 10
Comment 18 Alice0775 White 2011-05-17 17:03:36 PDT
Scroll lag:
http://www.youtube.com/watch?v=whep15wsL1k
Comment 19 Alice0775 White 2011-05-17 17:23:29 PDT
It is remarkable  if I hold down up/down arrow key.
Comment 20 Alice0775 White 2011-05-17 17:34:14 PDT
And also autoscroll.
Comment 21 Bas Schouten (:bas.schouten) 2011-05-17 18:03:38 PDT
Is this on a slow CPU maybe?
Comment 22 Alice0775 White 2011-05-17 18:05:44 PDT
(In reply to comment #21)
> Is this on a slow CPU maybe?

Maybe, Tesing with Core2Quad @2.5GHz
Comment 23 Bas Schouten (:bas.schouten) 2011-05-17 18:40:35 PDT
Hrm, I got a profile on this and CPU usage certainly seems to have increased since 4.0.1. But a profile doesn't really show too clearly where it is going, did my patch not make any difference for you? Or just some.
Comment 24 Bas Schouten (:bas.schouten) 2011-05-17 18:48:53 PDT
Something very odd is going on here. I can indeed get a slight perf increase (and significant CPU usage decrease) here, if I just push the actual clip and not optimize away some clipping work. I'm not actually sure what's going on here though, still investigating.
Comment 25 Alice0775 White 2011-05-17 18:58:11 PDT
(In reply to comment #23)
> Hrm, I got a profile on this and CPU usage certainly seems to have increased
> since 4.0.1. But a profile doesn't really show too clearly where it is
> going, did my patch not make any difference for you? Or just some.
In my impression,nothing changed.
Comment 26 Bas Schouten (:bas.schouten) 2011-05-17 19:16:13 PDT
I believe I understand the cause after having studied the profile in more detail, I'll probably add the following comment to outline my theory:

// XXX - This code still causes an interesting problem, if there
// is a clip chain of 2 clips. And we get a mask operation for
// the first clip first, and then another drawing operation for
// the second clip. We now having drawing operations for two
// 'clip levels' rather than one. This prevents direct2d from
// internally being able to prevent setting up stuff for clip
// level 1 and can actually hurt performance.
Comment 27 Bas Schouten (:bas.schouten) 2011-05-17 19:23:20 PDT
Created attachment 533148 [details] [diff] [review]
Part 2: Only optimize away clip levels when using a solid alpha mask

This brings another big improvement to this bug for me. Essentially this will only optimize away a clip level when using a solid alpha mask. This is the situation the optimization is really intended for (for optimizing transparent rectangular draws).
Comment 28 Alice0775 White 2011-05-17 22:53:48 PDT
(In reply to comment #27)
> Created attachment 533148 [details] [diff] [review] [review]
> Part 2: Only optimize away clip levels when using a solid alpha mask
> 
> This brings another big improvement to this bug for me. Essentially this
> will only optimize away a clip level when using a solid alpha mask. This is
> the situation the optimization is really intended for (for optimizing
> transparent rectangular draws).

FYI,
I tested in local build with patch of Part 2,
build from 5ffdf4967dec + attachment 533148 [details] [diff] [review]:
This works fine for me.
Comment 29 Asa Dotzler [:asa] 2011-05-18 23:28:13 PDT
Bas, would we need both of these patches in Firefox 5 to fix the regression? What's the risk there and how does that stack up against backing out Bug 600760?
Comment 30 Jeff Muizelaar [:jrmuizel] 2011-05-20 06:56:44 PDT
Comment on attachment 533148 [details] [diff] [review]
Part 2: Only optimize away clip levels when using a solid alpha mask

Please add more comments about why we need isSolidAlphaMask.
Comment 31 Daniel Veditz [:dveditz] 2011-05-23 15:13:49 PDT
Would still like an answer to Asa's question in comment 29 but tracking for now
Comment 32 Bas Schouten (:bas.schouten) 2011-05-23 15:22:09 PDT
(In reply to comment #29)
> Bas, would we need both of these patches in Firefox 5 to fix the regression?
> What's the risk there and how does that stack up against backing out Bug
> 600760?

We need both these patches to fix this page, some pages will be negatively affected by these patches(they might go back to FF4 perf), but I believe the majority positively. It's a complicated trade off, the second patch here is by no means a 'speed up' per sé, it just improves the worse-case scenario. This is the only page I've seen so far that's so negatively affected by the changed in bug 600760. We could take both these patches and the risk would be pretty small, but I'm not sure if the win is big enough, if we've got more bugs on this that I don't know about though, I could be wrong.

Backing out bug 600760 is worse I think because of us going back to being a lot worse at a bunch of canvas demo's. Benchmarks where some people have already blogged about our improvements.
Comment 33 Asa Dotzler [:asa] 2011-05-25 12:03:27 PDT
Comment on attachment 533010 [details] [diff] [review]
Change popping to only pop to common ancestor v2

Not a big enough reward to warrant the risk this late in the Beta cycle.
Comment 34 Asa Dotzler [:asa] 2011-05-26 14:48:40 PDT
I believe this made it onto the trunk before the migration to aurora 6. Am I wrong? If it didn't please nominate the patch(es). If I am, please unset the tracking request
Comment 35 Johnathan Nightingale [:johnath] 2011-06-02 14:35:15 PDT
(In reply to comment #34)
> I believe this made it onto the trunk before the migration to aurora 6. Am I
> wrong? If it didn't please nominate the patch(es). If I am, please unset the
> tracking request

Minusing based on radio silence - please re-nom where appropriate, if appropriate.
Comment 36 Bas Schouten (:bas.schouten) 2011-06-02 16:12:09 PDT
(In reply to comment #35)
> (In reply to comment #34)
> > I believe this made it onto the trunk before the migration to aurora 6. Am I
> > wrong? If it didn't please nominate the patch(es). If I am, please unset the
> > tracking request
> 
> Minusing based on radio silence - please re-nom where appropriate, if
> appropriate.

Ugh, sorry, my bad, I promised Blizzard I'd look at this yesterday. Yes, part 1 made it on before Aurora, but doesn't really solve this bug. Part 2 didn't.

I'll land part 2 on m-c tomorrow, I'm not sure if there's enough reward to merge it aurora though, we still haven't seen this problem on any other sites as far as I know.
Comment 37 Jim Jeffery not reading bug-mail 1/2/11 2011-06-21 05:58:34 PDT
Is it 'tomorrow' yet ?  now almost 3 weeks later ??
Comment 38 DB Cooper 2011-07-05 08:07:08 PDT
Has the second part landed on moz-central yet?
Comment 39 DB Cooper 2011-07-19 20:12:38 PDT
Ping.

Is this going to be closed? I still get "laggy" behaviour on some forum layouts, and I'm sure that many would appreciate an improvement. This is noticeable on my Xeon workstation.

Also, Gmail scrolling has become extremely slow on my AMD E-350 based netbook.
Comment 40 Harsh86 2011-10-02 04:57:25 PDT
Still slightly laggy in Firefox Nightly.
Comment 41 Alex 2011-10-02 08:23:38 PDT
Using D3D9 layers on W7 offers the best performance (smoothest scrolling), not D3D10/D2D. Seems like, instead of alleviating overhead, D3D10/D2D adds overhead. Before saying anything bad about D2D, IE9 works flawlessly and is much faster with D2D.
Comment 42 Bas Schouten (:bas.schouten) 2011-10-02 09:01:53 PDT
(In reply to Hera from comment #41)
> Using D3D9 layers on W7 offers the best performance (smoothest scrolling),
> not D3D10/D2D. Seems like, instead of alleviating overhead, D3D10/D2D adds
> overhead. Before saying anything bad about D2D, IE9 works flawlessly and is
> much faster with D2D.

Yes, the overhead here is in the cairo->Direct2D mapping. IE9 does not have that problem. Azure will longer-term alleviate some of this pain.

The second patch we could land, but I don't think it'll make a huge difference.
Comment 43 Alice0775 White 2011-10-02 09:21:51 PDT
See comment #28, at least , the 2nd patch will make improvement performance on the URL( http://piro.sakura.ne.jp/xul/xul.html.en ).
Comment 44 Joe Drew (not getting mail) 2011-12-10 11:46:54 PST
Bas, let's land part 2 on mozilla-central and close this bug. Future improvements will be due to the Azure project.
Comment 45 Marco Castelluccio [:marco] 2012-01-14 05:13:18 PST
Is the part 2 landing still needed?
Comment 46 Joe Drew (not getting mail) 2012-01-20 14:17:17 PST
I'm just going to call this checkin-needed, since Bas keeps ignoring us :)
Comment 48 Ed Morley [:emorley] 2012-01-23 11:46:36 PST
https://hg.mozilla.org/mozilla-central/rev/2c45818269df

Note You need to log in before you can comment on or make changes to this bug.