840431 - |text-overflow: ellipsis| shouldn't have any overhead

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Currently it costs about 10fps in scrolling the gaia email app.  See bug 840372.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I've done some mucking around with SAMPLE_LABEL and profiling. TextOverflow::ProcessLine is usually around 5% of the profile when scrolling through message headers in the Email app. We can get it down a bit (e.g. using the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=840372#c48, or being more aggressive and catching ellipsis metrics on the font), but at 5% it can't really be responsible for 10fps. I checked the actual painting of the ellipsis, that's around 0.5% (you'd expect it to be insignificant because only ellipses scrolled into view would be painted).

My current theory, from studying the display list dump in https://bugzilla.mozilla.org/show_bug.cgi?id=840372#c1, is that all the nested nsDisplayClips around TextOverflow ellipses are hurting us. We create all those nsDisplayClips because the ellipses are put on the PositionedDescendants list, and each item on that list is always independently clipped. Since the clips are nested 4 deep we're creating 5 display items per clip. This not only adds to the cost of display-list construction, but it also adds to the cost of ProcessDisplayItems in FrameLayerBuilder.

Bug 841192 should help here by eliminating nsDisplayClips. That change is trunk-only.

One way to eliminate the extra nsDisplayClips is to move the text-overflow ellipses into the content list. This reduces the size and complexity of the display lists a lot.

I can't really tell that this improves things in my build, but I get a lot lower performance in my build than Chris sees in bug 840372 comment #0, so there may be something strange about my build.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: possible fix

This would be for the B2G branch only. Bug 841192 will fix this for real.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> I can't really tell that this improves things in my build, but I get a lot
> lower performance in my build than Chris sees in bug 840372 comment #0, so
> there may be something strange about my build.

It's perhaps because my build is --enable-debug.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

:)

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This gets my build from 43-44fps to 46-47, so definitely an improvement.  Will try with attachment 717023 [details] [diff] [review] too.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

That takes us up to 47-48 with 49 popping up on occasion.  So these two patches together look like a 4-5fps win.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment 719334 [details] [diff] (which turns off 2/3 of the ellipsis displays per header) on top of that gets us to 48-49 with 50 relatively common, 51 on occasion.  So it's actually not much win on its own.

Is there something enabled by having /any/ text-overflow:ellipsis items in the display list that wouldn't run with /none/?

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Two wins rolled up

How does this feel on your devices?  This along with the gaia patch pushes scrolling to about 50fps on otoro.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #8)
> Is there something enabled by having /any/ text-overflow:ellipsis items in
> the display list that wouldn't run with /none/?

No.

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 719821 [details] [diff] [review]
possible fix

(you might need to disable some failing reftests too)

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> It's perhaps because my build is --enable-debug.

Maybe not ... removing B2G_DEBUG doesn't seem to have had much effect. Maybe it's central vs the b2g18 branch :-(.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Better ellipsis caching patch

This patch is a more aggressive than the previous ellipsis-caching patch. It stores a complete textrun in the gfxFontGroup so when you're scrolling down through a series of message headers we should get 100% cache hits, and the cache covers drawing of the ellipses as well as measuring them.

I need to pull and build the B2G18 branch to try to measure the perf impact.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> > It's perhaps because my build is --enable-debug.
> 
> Maybe not ... removing B2G_DEBUG doesn't seem to have had much effect. Maybe
> it's central vs the b2g18 branch :-(.

Unfortunately you need to clobber after flipping this flags :/.

I saw considerably better performance on m-c than on b2g18, ~5fps or so better on the patch with all the ellipses removed.

Comment 15

[Jonathan Kew [:jfkthame]]

Comment on attachment 720111 [details] [diff] [review]
Better ellipsis caching patch

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

Hmm... do we really change appUnitsPerDevPixel so frequently that we need to cache multiple ellipsis textruns for different appPerDev values? Seems like that might be overkill - would it be sufficient to cache a single ellipsis textrun in the group, and discard/replace it if the appPerDev value doesn't match? When scrolling a list in b2g email (for example), I'd expect that we're constantly reusing a textrun with the same appPerDev value.

Comment 16

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Comment on attachment 719831 [details] [diff] [review]
Two wins rolled up

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

I tried to analyze for following use case.
1) I loaded all emails in inbox from gmail.
2) Email Doesn't try to download any more email during scrolls.

TextOverflow:ProcessLine takes little less time with this patch (it takes ~5ms less time). But it doesn't create much difference in average fps. But if we consider a case where email app tries to download email while scroll is going on, then we still see stutters.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jonathan Kew (:jfkthame) from comment #15)
> Hmm... do we really change appUnitsPerDevPixel so frequently that we need to
> cache multiple ellipsis textruns for different appPerDev values?

No, but this way you can guarantee that a cached gfxTextRun will stay alive as long as the gfxFontGroup does, which simplifies things a lot.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry, didn't mean to clear that flag.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Tapas Kumar Kundu from comment #16)
> TextOverflow:ProcessLine takes little less time with this patch (it takes
> ~5ms less time). But it doesn't create much difference in average fps.

You mean the entire patch doesn't change fps? Chris said in comment #6 that it did.

Comment 20

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> (In reply to Jonathan Kew (:jfkthame) from comment #15)
> > Hmm... do we really change appUnitsPerDevPixel so frequently that we need to
> > cache multiple ellipsis textruns for different appPerDev values?
> 
> No, but this way you can guarantee that a cached gfxTextRun will stay alive
> as long as the gfxFontGroup does, which simplifies things a lot.

Could you expand on that a bit? It's not obvious to me why it would be hard to just have a single ellipsis textrun in the group, and use a getter that is always passed the appPerDev you want, so it knows whether it needs to refresh the run. Is it the mMarkerTextRun references in nsDisplayTextOverflowMarker that you're concerned may become invalid? Can't it hold a reference to the gfxFontGroup (or the nsFontMetrics), and just ask for the ellipsis textrun when needed?

I guess we can do the array of textruns if -really- necessary, but to me it seems like a bit of a sledgehammer for such a specialized thing. Convince me...?

Re the current patch, doesn't the gfxPangoFontGroup override of UpdateFontList() also need to clear any cached ellipsis textrun(s)?

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: rollup patch

Rolls up my gfxFont-based ellipsis caching patch, and the Content() display list patch.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jonathan Kew (:jfkthame) from comment #20)
> Could you expand on that a bit? It's not obvious to me why it would be hard
> to just have a single ellipsis textrun in the group, and use a getter that
> is always passed the appPerDev you want, so it knows whether it needs to
> refresh the run. Is it the mMarkerTextRun references in
> nsDisplayTextOverflowMarker that you're concerned may become invalid?

Yes.

> Can't it hold a reference to the gfxFontGroup (or the nsFontMetrics), and just ask
> for the ellipsis textrun when needed?

That's a bit more overhead and more code. It also makes the lifetime of the returned textrun a lot more complex; you'd have to say that the returned gfxTextRun is valid until the next call to GetEllipsisTextRun on this gfxFontGroup, or something like that. That sort of guarantee is nasty because it depends on you knowing exactly what the call graph is between acquiring and using the textrun.

> I guess we can do the array of textruns if -really- necessary, but to me it
> seems like a bit of a sledgehammer for such a specialized thing. Convince
> me...?

It's an implementation detail hidden behind the gfxFontGroup interface, and it's only a few lines of code, and it makes the lifetime of the returned gfxTextRun* simple --- "as long as you have a reference to the gfxFontGroup", which is easier to work with.

> Re the current patch, doesn't the gfxPangoFontGroup override of
> UpdateFontList() also need to clear any cached ellipsis textrun(s)?

Yes I think so. I'll add that to the trunk patch.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 721892 [details] [diff] [review]
rollup patch

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

tkundu, can you please try this and report whether the performance is acceptable? Thanks.

Comment 24

[Jonathan Kew [:jfkthame]]

Attachment: alternative - only cache a single ellipsis textrun, not an array of them

Alternative approach, as a patch on top of Roc's 'Better ellipsis caching patch'. This only caches a single ellipsis textrun in the font group, rather than an array of them. This version is actually slightly -less- code, not more. :) In theory it could be slightly less performant, as it requires getting the fontMetrics from the frame when drawing; is that really going to be an issue?

Comment 25

[Jonathan Kew [:jfkthame]]

Comment on attachment 722173 [details] [diff] [review]
alternative - only cache a single ellipsis textrun, not an array of them

I'm still pretty reluctant to put an array of ellipsis textruns into the font group just for this.

In the b2g mail app, I assume we'll always be using the same appUnitsPerDevPixel value, so there'll only be a single item in that array, but in a context where the user may zoom arbitrarily (e.g. a web page that uses text-overflow?), we might end up collecting quite a few textruns there, even though we only ever need one at a time. And then after the user has zoomed a few times, we'd be linear-searching through the obsolete ones each time we need the latest.

Something else we could consider would be to provide Draw() and GetAdvanceWidth() APIs on gfxShapedWord (like those on gfxTextRun, but simpler). Then we could cache and directly use gfxShapedWord objects here, which are much lighter-weight than gfxTextRun (as they don't need to support multiple glyph runs and styles). That would make the array-based cache somewhat more palatable.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jonathan Kew (:jfkthame) from comment #24)
> Alternative approach, as a patch on top of Roc's 'Better ellipsis caching
> patch'. This only caches a single ellipsis textrun in the font group, rather
> than an array of them. This version is actually slightly -less- code, not
> more. :) In theory it could be slightly less performant, as it requires
> getting the fontMetrics from the frame when drawing; is that really going to
> be an issue?

Not likely.

My main problem with this approach is that it has the strange textrun lifetime issue.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 722173 [details] [diff] [review]
alternative - only cache a single ellipsis textrun, not an array of them

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

But I'm OK with this.

Comment 28

[Jonathan Kew [:jfkthame]]

OK, let's try going with the single cached textrun.

However, there's an added complication (with either version of this patch), which is that they leak. The trouble is that gfxTextRuns hold a strong reference to their creating gfxFontGroup, so when we then cache a textrun in the fontgroup we've created a circular reference that prevents the fontgroup and textrun from ever being deleted, and hence we also keep font instances alive, etc.

I -think- we can fix this the "easy way" by just removing the NS_ADDREF/NS_RELEASE that the textrun does to its fontgroup - AFAICS, the textrun no longer needs to be responsible for ensuring its creating fontgroup stays alive. I suspect that was necessary with the older textrun-caching scheme, but textrun lifetime management is now simpler.

I've pushed a tryserver job with the addref/release pair removed, to see how that goes... https://tbpl.mozilla.org/?tree=Try&rev=c73f27412a87.

Comment 29

[Jonathan Kew [:jfkthame]]

Attachment: cache an ellipsis textrun on gfxFontGroup

Here's a version of the merged patch (i.e. mine folded into your original array-cache one) that also aims to deal with the leak, by not holding a reference back to the fontgroup from its cached ellipsis run. Pushed this to tryserver to see how it fares: https://tbpl.mozilla.org/?tree=Try&rev=a1fb4ad9d729.

Comment 30

[Jonathan Kew [:jfkthame]]

(FTR, removing the textrun's Addref/Release as per comment 28 didn't go well - in particular, a bunch of SVG tests failed - so I guess we do still need textruns to hold a reference to their fontgroup in general, to avoid premature deletion in some cases.)

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 722436 [details] [diff] [review]
cache an ellipsis textrun on gfxFontGroup

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

Please add the flushing code to gfxPangoFonts::UpdateFontList?

Comment 32

[Jonathan Kew [:jfkthame]]

I thought I'd done that? Just assigning null to the nsAutoPtr<> should do it.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 722436 [details] [diff] [review]
cache an ellipsis textrun on gfxFontGroup

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

oops

Comment 34

[Jonathan Kew [:jfkthame]]

Pushed the ellipsis-caching patch to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cfcf5473878

Roc, what about the earlier "possible fix" - is that still wanted for b2g18?

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4cfcf5473878

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 719821 [details] [diff] [review]
possible fix

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> Yes.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0cdbed626f6d
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/ebfa1ac15c69

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 722436 [details] [diff] [review]
cache an ellipsis textrun on gfxFontGroup

I believe we should take this patch on 18 as well. It should help a bit and shouldn't hurt anything.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll reopen the bug until we are sure this is fixed.

Also, it would help if you tell me exactly how to populate the email app and what gesture you use for testing frame rate. That might make a difference to performance.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> Comment on attachment 722436 [details] [diff] [review]
> cache an ellipsis textrun on gfxFontGroup
> 
> I believe we should take this patch on 18 as well. It should help a bit and
> shouldn't hurt anything.

You'll need to post a b2g18-specific patch for this one. It doesn't apply cleanly to the branch at all.

Also setting the bug's status flags back to affected based on comment 39. Please set them back to fixed when you're satisfied that everything's done here.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: cache ellipsis on gfxFontGroup --- b2g-18 patch

Comment 42

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39)
> Also, it would help if you tell me exactly how to populate the email app and
> what gesture you use for testing frame rate. That might make a difference to
> performance.

Give it the credentials of a valid IMAP or ActiveSync (ex: hotmail) account.  Let the account be created, continue to the inbox.  Wait for the synchronization process to complete; you can tell it's done when the progress bar under the header is done and goes away.  Note that it may show up a few times in sequence for IMAP.  Scroll up and down using a standard single-finger scrolling gesture.  A variation on infinite scrolling is supported where we start out buffering at least 2 screens worth of messages, but will retain up to 6.  As such, you may want to scroll a bit to make sure that we have retrieved the full 6 screens worth of messages so that your scrolling is demonstrative of just scrolling and no background processes.

We don't really have any canned accounts available that we can hand out, although Dave Hunt is working on some at least for automated testing.  Specifically, webmail services like gmail and hotmail notice when logins come from other countries/continents than they usually do and lock them down, so that tends to not work well.  Your mozilla.com account will work, although I wouldn't take the phone out into the world with those credentials still on it.  You can delete accounts from the settings screen, however, and/or reset the phone's storage.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 723760 [details] [diff] [review]
cache ellipsis on gfxFontGroup --- b2g-18 patch

https://hg.mozilla.org/releases/mozilla-b2g18/rev/542c6ad364ae
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/cdf058e47197

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll close this now and carry on in bug 846901, since I don't have any more optimizations to do on text-overflow:ellipsis specifically.

Comment 45

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/dc3add076016
Gaia   1e1c8c0ff2bc7f252fbe95016f108e38ece691a9
BuildID 20130314070204
Version 18.0
Unagi

Verified that ellipse do still occur and that it pans faster.

Comment 46

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Comment on attachment 721892 [details] [diff] [review]
rollup patch

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

looks like it is already verified by someone else. Please let me know if I neeed to verify it.