Closed Bug 840431 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla22
blocking-b2g tef+
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: cjones, Assigned: roc)

References

Details

Attachments

(5 files, 2 obsolete files)

Currently it costs about 10fps in scrolling the gaia email app.  See bug 840372.
Duplicate of this bug: 841671
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
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.
Depends on: 841192
Attached patch possible fixSplinter Review
This would be for the B2G branch only. Bug 841192 will fix this for real.
Attachment #719821 - Flags: review?(matspal)
(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.
This gets my build from 43-44fps to 46-47, so definitely an improvement.  Will try with attachment 717023 [details] [diff] [review] too.
That takes us up to 47-48 with 49 popping up on occasion.  So these two patches together look like a 4-5fps win.
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/?
Attached patch Two wins rolled up (obsolete) — Splinter Review
How does this feel on your devices?  This along with the gaia patch pushes scrolling to about 50fps on otoro.
Attachment #719831 - Flags: feedback?(mvines)
(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 on attachment 719821 [details] [diff] [review]
possible fix

(you might need to disable some failing reftests too)
Attachment #719821 - Flags: review?(matspal) → review+
Attachment #719831 - Flags: feedback?(mvines) → feedback?(tkundu)
(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 :-(.
Attached patch Better ellipsis caching patch (obsolete) — Splinter Review
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.
Attachment #720111 - Flags: review?(jfkthame)
(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 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.
blocking-b2g: --- → tef+
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.
(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.
blocking-b2g: tef+ → ---
Sorry, didn't mean to clear that flag.
blocking-b2g: --- → tef+
(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.
(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)?
Attached patch rollup patchSplinter Review
Rolls up my gfxFont-based ellipsis caching patch, and the Content() display list patch.
Attachment #719831 - Attachment is obsolete: true
Attachment #719831 - Flags: feedback?(tkundu)
(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 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.
Attachment #721892 - Flags: feedback?(tkundu)
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?
Assignee: nobody → 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.
Attachment #722173 - Flags: feedback?(roc)
(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 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.
Attachment #722173 - Flags: feedback?(roc) → feedback+
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.
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.
Attachment #722436 - Flags: review?(roc)
(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 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?
Attachment #722436 - Flags: review?(roc)
I thought I'd done that? Just assigning null to the nsAutoPtr<> should do it.
Attachment #722436 - Flags: review?(roc)
Comment on attachment 722436 [details] [diff] [review]
cache an ellipsis textrun on gfxFontGroup

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

oops
Attachment #722436 - Flags: review?(roc) → review+
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?
https://hg.mozilla.org/mozilla-central/rev/4cfcf5473878
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Attachment #720111 - Attachment is obsolete: true
Attachment #720111 - Flags: review?(jfkthame)
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.
Attachment #722436 - Flags: checkin?
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #722436 - Flags: checkin?
(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.
(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.
Attachment #719821 - Flags: checkin+
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.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
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.
Attachment #721892 - Flags: feedback?(tkundu) → feedback+
You need to log in before you can comment on or make changes to this bug.