Email scrolling fps is too low: Valid region gets too complicated

VERIFIED FIXED in Firefox 21

Status

Firefox OS
General
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

unspecified
B2G C4 (2jan on)
x86_64
Linux
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:tef+, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [target 28/2])

Attachments

(11 attachments, 2 obsolete attachments)

43.68 KB, text/plain
Details
1.15 KB, patch
asuth
: review+
Details | Diff | Splinter Review
1.14 KB, patch
roc
: review+
Details | Diff | Splinter Review
41.96 KB, image/png
Details
43.14 KB, image/png
Details
42.60 KB, image/png
Details
47.77 KB, image/png
Details
855 bytes, patch
asuth
: review+
Details | Diff | Splinter Review
276 bytes, text/html
Details
11.69 KB, patch
mats
: review+
Details | Diff | Splinter Review
11.75 KB, patch
Details | Diff | Splinter Review
STR
 (1) Sync messages from server
 (2) Scroll in messages

We stay around 44-45 fps, but the target is 60.  This is mostly likely because we're missing the 16ms target every fourth frame, so the compositor is drawing frames at 16ms, 16ms, 32ms intervals.

Here's a profile

http://people.mozilla.com/~bgirard/cleopatra/#report=8c2197d97554851130b50264606b9df8a9c8d05c

No low-hanging fruit presents itself; we're just spending too much time painting.  The usual suspects are standing out
 - 23.7%: display list construction
 - 21.9%: blocked on compositor
 - 19.6%: painting layer tree
 - 15.2%: ContainerState::ProcessDisplayItems
Created attachment 712735 [details]
Display list

Nothing particularly jumps out again, except that I see two nsDisplayTransforms in the optimized list.  I wonder if email is trying to hide a screen but not quite succeeding ...

We also have a thebeslayer with a pretty complex valid region.
Created attachment 712746 [details] [diff] [review]
Remove some stuff

With this change I get scrolling up to the display refresh rate.
Created attachment 712751 [details] [diff] [review]
Disable text-overflow: ellipsis

This is the minimal change that gets us up to ~display refresh rate.  roc tells me that ellipsis requires a second pass over the display items that use it.

However, there's a second issue: the scrolling fps has kind of a bimodal distribution.  On first launch of the email app, it's low.  But if I send email into the background (throwing away its layer tree), it gets up near the display refresh rate.  This is really odd, seems like either failure to get gralloc or maybe us carrying around a too-complex valid region (see above).
Attachment #712746 - Attachment is obsolete: true
I'm surprised ellipsis processing makes that much of a difference!
I think this would be a lot faster if we got rid of all those nsDisplayClips. Many of them aren't even clipping anything since the clip bounds contains the bounds of their child(ren). E.g.:

            Clip 0x462110e0() (0,0,19200,27600)(0,0,0,0)(0,0,0,0)
              Clip 0x462110e0() (0,3180,19200,27600)(0,0,0,0)(0,0,0,0)
                Clip 0x462110e0() (0,13020,19200,4800)(0,0,0,0)(0,0,0,0)
                  Clip 0x462110e0() (2280,14850,13920,1200)(0,0,0,0)(0,0,0,0)
                    TextOverflow 0x462110e0() (15000,14850,720,1200)(0,0,0,0)(0,0,0,0) layer=0x43c9b400

What I've wanted to do for a while is add a clip rect field to nsDisplayItem, and an optional pointer to a rounded-rect-clipping object, and eliminate nsDisplayClip/nsDisplayClipRoundedRect altogether.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> But if I send
> email into the background (throwing away its layer tree), it gets up near
> the display refresh rate.  This is really odd, seems like either failure to
> get gralloc or maybe us carrying around a too-complex valid region (see
> above).

Correction: we keep the layer tree around, but just toss out the buffers.  So that further suggests a too-complex valid region.
That valid region is crazy! We should definitely fix that in ThebesLayerOGL, we don't seem to have any complexity throttling there right now.

We could do a mValidRegion.SimplifyInward() after each change to mValidRegion. That will wipe out the region if it gets too complex, maybe we should do something smarter...
This is shadow layers, so we're in BasicShadowableThebesLayer.  But I don't see us simplifying the valid region there either.  Will see if this can be fixed with a surgical strike.
Oh my ... we somehow end up with a thebes layer valid region of up to 90 rects (88 in the steady state).  Throwing out the buffers is what fixes that.
Created attachment 712829 [details] [diff] [review]
Disable |text-overflow: ellipsis|

This patch and the next win us 10fps or so when scrolling in the email view.
Attachment #712751 - Attachment is obsolete: true
Attachment #712829 - Flags: review?(bugmail)
Created attachment 712830 [details] [diff] [review]
Don't let valid regions grow beyond 8 rects

I chose 8 because ... uh ... that's what I chose.  Not sure how to make a better decision.

This makes the degenerate valid region on email startup (before buffer toss) go away.
Attachment #712830 - Flags: review?(roc)
These patches don't get us to ~display refresh rate on b2g18 testing on otoro.  We're maxing out around 52-53fps.  So yay, we've improved scrolling on inbound.

With some boosts on test targets we should be closer to refresh rate.
Comment on attachment 712829 [details] [diff] [review]
Disable |text-overflow: ellipsis|

Woooo!  Thanks for the fix!

In terms of the clip regions, should we be throwing visibility: hidden on off-screen cards or something like that?
Attachment #712829 - Flags: review?(bugmail) → review+
Off-screen cards won't appear in the (unoptimized) display list so they shouldn't be creating overhead here.
Needed to hit another product target.
blocking-b2g: --- → tef?
Dropping the ellipsis doesn't make me particularly happy, but
 - buys us 10fps scrolling
 - shows more text in a very limited space (bug 840397)
Flags: needinfo?(jcarpenter)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d47438ef23c1

Leaving open for UX feedback before landing email patch.
Whiteboard: [leave open]
(In reply to Chris Jones [:cjones] [:warhammer] from comment #16)
> Dropping the ellipsis doesn't make me particularly happy, but
>  - buys us 10fps scrolling
>  - shows more text in a very limited space (bug 840397)

Sounds great to me, but we should see a screenshot of the new treatment before signing off. Also adding Casey for needsinfo since this app is his baby.
Flags: needinfo?(jcarpenter) → needinfo?(kyee)
The timestamps don't quite match up because of comment 18 ;), but you can compare starting from below there.
Thanks Chris. Unfortunately I think we're getting into baby-with-bath water territory here, with legibility problems created by the cut-off characters in the Subject and Body excerpts and the collision of the email address and time stamp on their single line. It winds up making the app look very rough. 

* What happens if we add the ellipsis back to the email address but leave it off the Subject and Body?
* Is it feasible to try a different approach to truncation? On the home screen we do an alpha fade on app names, for example.
Created attachment 713310 [details]
After, v2

This costs us 4-5fps on scrolling which is going to drop us under the target.
Flags: needinfo?(jcarpenter)
(In reply to Josh Carpenter [:jcarpenter] from comment #22)
> * Is it feasible to try a different approach to truncation? On the home
> screen we do an alpha fade on app names, for example.

I'm not sure how the homescreen does that, but I'm pretty certain that's going to be more expensive than overflow:ellipsis in the email use case.

Any other suggestions for alternative implementations?
Flags: needinfo?(21)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #24)
> Any other suggestions for alternative implementations?

We could manually ellipsercise the text in JS and take the hit at DOM insert time instead.  Binary search on string length using either Canvas.measureText or just by twiddling the DOM and triggering (hopefully tiny) re-flows.  We would cache our results up a bit.
Er, and I guess another variant would be instead of putting the ellipse in the text string, we just have a DOM element that only contains ellipsis that we make visible on overflow.  I think that's how the XUL overflow stuff works/worked?  I am told we have an overflow event too...
Created attachment 713560 [details]
Can we fix the margins while at it?

Ugh. This looks like a bug now. Can we please fix the gutter spacing as well. I was hoping for v.1.1, but without the contact photos and ellipsis with the current spacing, it looks so broken. See Screenshot.
30px from the left (it the text should be vertically aligned with string "Inbox" in the header).
30px from the right.

Vertically centered within the margins are the unread dot and attachment icons.
Flags: needinfo?(bugmail)
Also! Can we use "min" instead of "minutes"?
(In reply to Patryk Adamczyk [:patryk] UX from comment #28)
> Created attachment 713560 [details]
> Can we fix the margins while at it?
> 
> Ugh. This looks like a bug now. Can we please fix the gutter spacing as
> well. I was hoping for v.1.1, but without the contact photos and ellipsis
> with the current spacing, it looks so broken. See Screenshot.
> 30px from the left (it the text should be vertically aligned with string
> "Inbox" in the header).
> 30px from the right.
> 
> Vertically centered within the margins are the unread dot and attachment
> icons.

Bug 840397 which you have commented on is dealing with this stuff.  :epang is assigned.

(In reply to Patryk Adamczyk [:patryk] UX from comment #29)
> Also! Can we use "min" instead of "minutes"?

See bug 829213 for some context; we are using the l10n shared date logic for this in:
https://github.com/mozilla-b2g/gaia/blob/master/shared/js/l10n_date.js#L82
The strings come from here:
https://github.com/mozilla-b2g/gaia/blob/master/shared/locales/date/date.en-US.properties#L47

I'm unaware of an existing l10n bug for shortening those strings, but one could very well exist.  If you find/create one, please chime in on bug 829213 or cc me.
Flags: needinfo?(bugmail)
Comment on attachment 712830 [details] [diff] [review]
Don't let valid regions grow beyond 8 rects

We're creeping scope here on the email change, so let's split out the platform fix from gaia.

This is a low-risk change that prevents gecko from getting itself into a degenerate state where layers are very expensive to paint and composite.  This generate state cost email panning 4-5fps.  The worst regression we could see from this is overpainting.  Manual testing was completed on core gaia apps and no performance regressions observed.
Attachment #712830 - Flags: approval-mozilla-b2g18?

Comment 32

5 years ago
I feel we really need the ellipsis here.   Looks broken without them.    Would using some kind gradient so that it looks like the text fades off perform any better?
Flags: needinfo?(kyee)
I don't know how the homescreen implements that (ni? -> vivien), but I'm doubtful that would perform better.  Most likely worse.
You could stick a PNG with a transparent gradient over the text. That should be fast.
I kinda wonder why the text-overflow processing makes such a big difference. I'll try looking into that.
(In reply to Andrew Sutherland (:asuth) from comment #30)
> I'm unaware of an existing l10n bug for shortening those strings, but one
> could very well exist.  If you find/create one, please chime in on bug
> 829213 or cc me.

Kaze and I worked out l10n for compact time stamps in the Notifications Center, FWIW.
Flags: needinfo?(jcarpenter)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #23)
> Created attachment 713310 [details]
> After, v2
> 
> This costs us 4-5fps on scrolling which is going to drop us under the target.

There are few things I love more in life than smooth scrolling, but FPS targets don't matter for much if the layout is a mess. Are these hard partners requirements? 

(In reply to Andrew Sutherland (:asuth) from comment #27)
> Er, and I guess another variant would be instead of putting the ellipse in
> the text string, we just have a DOM element that only contains ellipsis that
> we make visible on overflow.  I think that's how the XUL overflow stuff
> works/worked?  I am told we have an overflow event too...

Of the two approaches you suggest, are either feasible for the same release as removal of text-overflow:ellipsis? If not, I don't think it's worth it unless there's a very hard partner requirement.

Definitely want a have-our-cake-and-eat-it-too scenario, but if it's not possible, would prefer to defer FPS improvement.
(In reply to Josh Carpenter [:jcarpenter] from comment #37)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #23)
> > Created attachment 713310 [details]
> > After, v2
> > 
> > This costs us 4-5fps on scrolling which is going to drop us under the target.
> 
> There are few things I love more in life than smooth scrolling, but FPS
> targets don't matter for much if the layout is a mess. Are these hard
> partners requirements? 

Similarly, if the fps is junk the layout is irrelevant.  (Mozilla has learned that painfully several times.)  We're at neither extreme here, so let's find a fixed point, including partner input :).
Created attachment 714302 [details] [diff] [review]
Email patch used to create "After, v2" screenshot

Haven't tested much to see if this is a win in and of itself, but presuming r+ based on earlier patch.
Attachment #714302 - Flags: review+
(In reply to Josh Carpenter [:jcarpenter] from comment #37)
> Of the two approaches you suggest, are either feasible for the same release
> as removal of text-overflow:ellipsis? If not, I don't think it's worth it
> unless there's a very hard partner requirement.

All of the approaches are reasonably easy to implement and to quantify the up-front cost.  I would ideally like to wait for:

1) roc to see what's up with overflow: ellipsis slowing us down in the first place.  Since it seems like layout is able to much more efficiently calculate anything we could do from JS and cache it, just having that work right and fast is ideal.  Of course, if it's a complex fix, our JS workarounds would be much easier to make happen.

2) an explicit prioritization by getting leo+ or what not on it.
We can't approve this perf win without weighing it against the UX regression. ni? on the product team.
Flags: needinfo?(ffos-product)

Comment 42

5 years ago
From Product Team perspective, I would like to know:

1> Are attachments in comment 19 and 23 the 2 options? (visually) or is there one more after the subsequent comments? 
2) Best is if we can lay down all options mapped with visuals and perf numbers (can we please?

Once we have #2, I can get with Josh to make a call from Product perspective.

Sandip

Comment 43

5 years ago
Some additional input:

We need to make a balanced decision here.  Both the UI layout and the fps are important -- none of these are partner specific asks that I am aware of, but will be bugs they file if they're below our perf targets.  

I'd recommend we wait to hear back from roc and see what's the root cause here.  If we need to take a hit, I'd defer to UX on the areas which we *must* have ellipsis and cut out the other areas for v1.
Flags: needinfo?(ffos-product)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #33)
> I don't know how the homescreen implements that (ni? -> vivien), but I'm
> doubtful that would perform better.  Most likely worse.

Cristian is the guy that dit it so let have him explained.
Flags: needinfo?(21) → needinfo?(crdlc)
Hi all, 

   I implemented an SVG mask [1] and it [2] is used for app names larger than its container. Maybe the performance is better defining the svg in html markup and not as external resource.

Thanks

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/style/app_name_mask.svg
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/style/grid.css#L79
Flags: needinfo?(crdlc)
Roc: can you weigh in on the question in comment 43?
Flags: needinfo?(roc)
No longer blocks: 837218
Created attachment 716991 [details]
testcase designed to stress text-overflow:ellipsis on small blocks

This test stresses what I think we're hitting on the email app: a number of one-line blocks with text-overflow:ellipsis.

Profile of scrolling this page shows that most of the work related to text-overflow is in Marker::SetupString. This runs once per block and it's a bit expensive because we create a rendering context and re-measure the ellipsis every time we paint each block.
Flags: needinfo?(roc)
Created attachment 717023 [details] [diff] [review]
cache ellipsis width

Someone should try this patch on B2G to see if it solves the text-overflow performance issue.
Attachment #717023 - Flags: review?(matspal)
Comment on attachment 717023 [details] [diff] [review]
cache ellipsis width

LGTM, r=mats

One further optimization that I think we can do for a default ellipsis
is to call
nsFontMetrics::GetWidth(const char* aString, uint32_t aLength,
                        nsRenderingContext *aContext)
directly instead of nsLayoutUtils::GetStringWidth.
This would bypass bidi processing which shouldn't be necessary for
0x2026 or "...".

Even further, we could move both the cached width and the GetEllipsis
lookup function to the font itself, it already has 'spaceWidth' and
'zeroOrAveCharWidth' - we could add an 'ellipsisWidth' there.
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.h#1339
http://mxr.mozilla.org/mozilla-central/source/layout/generic/TextOverflow.cpp#28
Attachment #717023 - Flags: review?(matspal) → review+
(In reply to Mats Palmgren [:mats] from comment #49)
> One further optimization that I think we can do for a default ellipsis
> is to call
> nsFontMetrics::GetWidth(const char* aString, uint32_t aLength,
>                         nsRenderingContext *aContext)
> directly instead of nsLayoutUtils::GetStringWidth.
> This would bypass bidi processing which shouldn't be necessary for
> 0x2026 or "...".

True, though it's probably not that important as long as this cache is used.

> Even further, we could move both the cached width and the GetEllipsis
> lookup function to the font itself, it already has 'spaceWidth' and
> 'zeroOrAveCharWidth' - we could add an 'ellipsisWidth' there.
> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.h#1339
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/TextOverflow.
> cpp#28

I discussed that with Jonathan Kew and decided it was a bit ugly and unnecessary to put ellipsis-specific code into gfxFont.
blocking as per comment 15 - partner req
blocking-b2g: tef? → tef+
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → affected
Andrew, can you try roc's patch on B2G?
Assignee: nobody → bugmail
(In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> Comment on attachment 712830 [details] [diff] [review]
> Don't let valid regions grow beyond 8 rects
> 
> We're creeping scope here on the email change, so let's split out the
> platform fix from gaia.
> 
> This is a low-risk change that prevents gecko from getting itself into a
> degenerate state where layers are very expensive to paint and composite. 
> This generate state cost email panning 4-5fps.  The worst regression we
> could see from this is overpainting.  Manual testing was completed on core
> gaia apps and no performance regressions observed.

Wow, we're still sitting on this.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #48)
> Created attachment 717023 [details] [diff] [review]
> cache ellipsis width
> 
> Someone should try this patch on B2G to see if it solves the text-overflow
> performance issue.

We really need to get you guys set up to do this yourselves :).  Feel free to ping around #b2g.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #48)
> Created attachment 717023 [details] [diff] [review]
> cache ellipsis width
> 
> Someone should try this patch on B2G to see if it solves the text-overflow
> performance issue.

Might win back a couple of fps but it's not a big difference.
I ran with the gecko-18 patch provided by cjones using a freshly built build.

git log -1 says:
commit baee79387d0a5205489467a9b78f5d6f183283fb
Author: Jonathan Griffin <jgriffin@mozilla.com>
Date:   Wed Feb 20 11:25:35 2013 -0800


Without the patch, I see FPS updates around 30fps or I see updates around 40fps.  When I first start the app, I get 30/low 30's.  Then if I hold down the home-screen button so the app zooms out and I pick the e-mail app again.  FPS is then 40/low 40's.

With the patch, the initial FPS numbers are similar, although the seem to dip into the high 20's more, but the reactivity feels much worse.  After holding down the home-screen button and then reselecting the app, I'm back into the 40's and I see 45 and 47 much more often but variability still seems higher.  If I stop touching the screen, the FPS stabilizes on constantly oscillating between 28/29/30.


Is there a way to get the FPS app to dump telemetry-like histograms or the list of frame times in milliseconds like the tech report uses for video card reviews?
http://techreport.com/review/21516/inside-the-second-a-new-look-at-game-benchmarking/2
Assignee: bugmail → nobody
Whiteboard: [leave open] → [leave open][target 28/2]
Assignee: nobody → jones.chris.g
Comment on attachment 712830 [details] [diff] [review]
Don't let valid regions grow beyond 8 rects

Clearing the approval flag as this is a tef+
Attachment #712830 - Flags: approval-mozilla-b2g18?
Let's go ahead and uplift the valid region fix, and sort out the UX logjam in bug 846187.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Summary: Email scrolling fps is too low → Email scrolling fps is too low: Valid region gets too complicated
Whiteboard: [leave open][target 28/2] → [target 28/2]
Man, this bug really needs to make use of "Mark patch as obsolete" ...

https://hg.mozilla.org/releases/mozilla-b2g18/rev/a3f1ceead7ce
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/3867fca22e3d
status-b2g18: affected → fixed
status-b2g18-v1.0.1: affected → fixed
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
Target Milestone: --- → B2G C4 (2jan on)

Updated

5 years ago
Flags: in-moztrap-

Comment 62

5 years ago
Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 63

5 years ago
issue appears fixed in V1.0.1 and V1.1.0
- The target of 60 fps was met. 

tested on Unagi running the following builds:
V1.0.1
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/ccec751a468e
Gaia   ee0bef61c0a25c806dd1eec5a4e047bc418a5f73
Build  2013-04-02-070202

V1.1.0
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/68c8a883cfc0
Gaia   1c38c91bb16f2bf0d5066c4787d2249463f61bb3
Build  2013-04-02-070204
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.