Last Comment Bug 681867 - Scrolling a GMail email conversation is really slow and chunky
: Scrolling a GMail email conversation is really slow and chunky
Status: VERIFIED FIXED
[qa!]
: verified-beta
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla10
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
: Jet Villegas (:jet)
Mentors:
: 695350 (view as bug list)
Depends on:
Blocks: 312156
  Show dependency treegraph
 
Reported: 2011-08-24 22:08 PDT by Ben Turner (not reading bugmail, use the needinfo flag!)
Modified: 2011-11-24 13:39 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
?
affected
fixed


Attachments
Part 1: fix text-overflow issues (4.26 KB, patch)
2011-10-19 22:01 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
part 2: remove nsDisplayForcePaintOnScroll (2.24 KB, patch)
2011-10-19 22:14 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
improved part 1 (6.88 KB, patch)
2011-10-20 16:24 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
improved v2 (6.94 KB, patch)
2011-10-20 16:27 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Ben Turner (not reading bugmail, use the needinfo flag!) 2011-08-24 22:08:04 PDT
I don't know if GMail has done something new (the floating top bar seems new) or if I'm just noticing this because I switched to Windows for my primary machine a few days ago, but scrolling a GMail email conversation is really bad. It's slow, jerks, lags way behind my mouse, and it seems to chew CPU. This new machine of mine has a pretty good graphics card and everything claims to be fully accelerated... Disabling the acceleration doesn't seem to make any difference.

Opening Chrome and scrolling is smooth and fast. What are we doing differently?
Comment 1 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-08-25 19:10:08 PDT
Interesting... When the conversation has only a few emails in it scrolling isn't really bad.

When you have a lot of emails in one conversation GMail will group them all together and just show little lines for each message. Scrolling in this stage is worse than with only a few messages but not terrible.

Then when you click on the collection of lines they expand to show subject lines. Here scrolling is really awful.

As you click each subject line to expand the message scrolling seems to get better.

It's got to be something funny with the way they do those collapsed message subject line headers?
Comment 2 :aceman 2011-10-19 08:28:49 PDT
Well, can we dupe this against bug 579260 or is there something specific in this bug report?
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-19 16:19:12 PDT
Comment #1 is useful. I want to work on this.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-19 19:25:01 PDT
Hmm, has this gotten better by itself?
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-19 20:35:19 PDT
I didn't get numbers before, but it feels a little better to me than it used to. But here are some real numbers:

I used this bookmarklet in Firefox and Chrome:

var startTime=Date.now(); var iters = 0; function doScroll() { var w = document.getElementById("canvas_frame").contentWindow; w.scrollTo(0, w.scrollY==200 ? 210 : 200); if (iters++ < 100) setTimeout(doScroll, 0); else alert(w.innerWidth + "x" + w.innerHeight + ": " + (Date.now() - startTime) + "ms"); } doScroll();

Content window for both browsers 1543x816

Scrolling dev.planning folder view:
Firefox trunk: 3050ms
Chrome: 1650ms

Scrolling "a five week release cycle?" thread view (nothing expanded):
Firefox trunk: 9750ms
Chrome: 2600ms

Scrolling "a five week release cycle?" thread view (subjects expanded):
Firefox trunk: 9200ms
Chrome: 6260ms

There's an additional problem where transitioning from "scrolling headers" to "fixed headers" as you scroll down is janky in Firefox. I'll look at that later.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-19 21:07:23 PDT
I profiled "scrolling dev.planning folder view".

21% of the time under InvalidateFixedBackgroundFrames, called during every scrollTo. This builds a display list, computes visibility, then walks the list to see if there's anything visible that needs to be repainted whenever we scroll. I'm not sure what's triggering this in GMail ... it could be either text-overflow or background-attachment:fixed. We should be able to eliminate this somehow.

33% of the time is under FrameLayerBuilder::DrawThebesLayer, painting ThebesLayer contents. This seems excessive given we're only scrolling by 10 pixels. Maybe there's invalidation being caused by InvalidateFixedBackgroundFrames.

About 24% of the time is display list construction and layer building during painting.

Altogether 62% of the time is painting. About 5% compositing layers and other misc overhead.

The rest of the profile is frittered away on script execution, some event handlers, some random unassigned driver work, and various other overhead.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-19 22:01:53 PDT
Created attachment 568300 [details] [diff] [review]
Part 1: fix text-overflow issues

SetHasFixedBackgroundFrame was being called for text-overflow elements (the label names in GMail's left-hand pane), which was forcing us to do those expensive InvalidateFixedBackgroundFrames and then forcing us to do unnecessary painting.

All we need to do for text-overflow is ensure that when the text-overflow's scrolling block is scrolled, the contents of the block are repainted, so let's do that explicitly using a state bit on the scrollframe.

This improves "scrolling dev.planning folder view" to 1550ms. I'm declaring victory on that one.
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-19 22:12:27 PDT
This improves "a five week release cycle? thread view (nothing expanded)" to 2580ms.

It improves "a five week release cycle? thread view (subjects expanded)" to about 1800ms.

I'll look into the "nothing expanded" case, we're already about as fast as Chrome there with the patch, but I'd like to know why it's slower than with subjects expanded ... it should be displaying less actual content.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-19 22:14:16 PDT
Created attachment 568304 [details] [diff] [review]
part 2: remove nsDisplayForcePaintOnScroll

No longer needed.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-19 22:33:20 PDT
In the "nothing expanded" case, we're spending a significant amount of time painting linear gradients, but I can't yet see what the gradient is being used for.
Comment 11 :aceman 2011-10-20 00:04:34 PDT
OK, thanks for explaining.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-20 15:34:44 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> This improves "a five week release cycle? thread view (nothing expanded)" to
> 2580ms.

Retesting this morning, I'm getting more like 1800ms (quite variable, but always under 2000ms). So I'm declaring victory on this entire bug.
Comment 13 Mats Palmgren (:mats) 2011-10-20 15:55:14 PDT
Comment on attachment 568300 [details] [diff] [review]
Part 1: fix text-overflow issues

Please update the DidProcessLines() doc comment in TextOverflow.h
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-20 16:03:22 PDT
I filed bug 696248 on the last issue in comment #5.
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-20 16:24:35 PDT
Created attachment 568553 [details] [diff] [review]
improved part 1

I realized we actually don't need DidProcessLines at all now.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-20 16:27:25 PDT
Created attachment 568556 [details] [diff] [review]
improved v2

Oops, that one didn't build.
Comment 17 Mats Palmgren (:mats) 2011-10-20 17:03:34 PDT
Comment on attachment 568556 [details] [diff] [review]
improved v2

Please update the class doc comment in TextOverflow.h
Comment 20 Ryan VanderMeulen [:RyanVM] 2011-10-21 17:58:03 PDT
roc, it appears you landed the original v2 patch instead of the improved one?
Comment 21 Mats Palmgren (:mats) 2011-10-21 18:07:07 PDT
Looks correct to me.
Comment 22 Ryan VanderMeulen [:RyanVM] 2011-10-21 18:08:54 PDT
Whoops, was equating v2 with part 2. Sorry for the spam.
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-22 02:14:23 PDT
Comment on attachment 568556 [details] [diff] [review]
improved v2

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

Requesting Aurora approval. This fixes a performance regression due to text-overflow landing. It affects at least one very important site. The fix is very low risk ... any regression is likely to involve scrolling in an element with text-overflow, which is completely broken in most other browsers already.
Comment 24 David Baron :dbaron: ⌚️UTC-10 2011-10-24 08:22:55 PDT
*** Bug 695350 has been marked as a duplicate of this bug. ***
Comment 25 Mike Beltzner [:beltzner, not reading bugmail] 2011-10-31 20:50:34 PDT
This has been missed in triage a couple of times, and a dupe has been filed. It's a highly visible site to users, feels like big win and Roc's assertion of low risk in comment 23 gives me warm fuzzy feelings - any reason why triage has passed it by a few times?
Comment 26 christian 2011-10-31 23:05:34 PDT
Comment on attachment 568556 [details] [diff] [review]
improved v2

[triage comment]

Thanks for flagging. We slogged through all the beta stuff as it was higher priority but all those lists are back to zero so now we're on aurora :-)

Approved for aurora9, this looks like a great fix. Thanks!
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-01 19:54:02 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/90a4c98c1ae3
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-01 19:56:06 PDT
This regression affects Firefox 8 as well. Don't suppose we can still get this in there?
Comment 29 juan becerra [:juanb] 2011-11-03 09:20:32 PDT
Adding [qa+] to make sure it gets verified where it lands. Looks like a high impact bug that can be verified easily.
Comment 30 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-24 13:39:43 PST
Seems to work well in Firefox 9.0b3. Marking verified.

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