967292 - [Contacts] nsDiplayText overhead is too high

Status: RESOLVED WORKSFORME

(Core :: Graphics: Text, defect, P1)

Description

[Benoit Girard (:BenWa)]

While scrolling the contacts app we spend a lot of time in nsDisplayText. We spend very little time actually composting the glyph and instead spend a lot of time shaping and building the text runs and general overhead. This accounts for most of our painting budget.

Profile:
http://people.mozilla.org/~bgirard/cleopatra/#report=eb664ce6b5319040d4dc8d9c7ae3ea299cb5adbb&search=nsDisplayText

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I was pretty sure we had caching for all of this; is that still in use/being hit?

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Jet, can you find somebody to take a quick look at this, we want to see if it can be done in 28 (just entering beta).

Comment 3

[Jet Villegas (inactive)]

Jonathan, can you take a look to see if the shaped word caches are in effect for this case?

Comment 4

[Jonathan Kew [:jfkthame]]

BenWa, can you explain how to view that profile and see the relevant stuff? When I try to load it, I'm not seeing anything about shaping or building textruns .... but this is probably because I don't know my way around the UI. Help?

Comment 5

[Benoit Girard (:BenWa)]

(In reply to Jonathan Kew (:jfkthame) from comment #4)
> BenWa, can you explain how to view that profile and see the relevant stuff?
> When I try to load it, I'm not seeing anything about shaping or building
> textruns .... but this is probably because I don't know my way around the
> UI. Help?

That's because the 'Compositor' thread is select by default which doesn't spend any time in nsDisplayText. You need to click on the GeckoMain timeline. I'll attach a quick video demo.

Comment 6

[Benoit Girard (:BenWa)]

Attachment: cleodemo.gif

Comment 7

[Jonathan Kew [:jfkthame]]

Huh, that's pretty much what I thought, but I couldn't get it to co-operate. Seems fine now, though. Thanks!

So looking at this, one thing that jumps out is that we're using gfxFont::ShapeTextWithoutWordCache. This means we're not caching the shaped glyphs for each fragment of text we see, but re-shaping every time the same string (in the same font) appears.

Looking into the Fira Sans font (which I assume is what's being used here), I can see the reason for this: the font includes an alternate <space> glyph that's used when the automatic-fractions feature is applied, to provide a "thin space" in sequences like "1 1/2" (when rendered as "1½"). That's all very well, but the existence of OpenType features that involve <space> disables our word caching (as per bug 761442).

What this implies is that throughout FirefoxOS, wherever we're using the Fira Sans font, we're losing out on shaped-word caching. That's pretty sad.

There are a few possible options we could consider to mitigate this:

(1) Remove that particular feature from the Fira Sans fonts. It's likely to be very rarely used, so it wouldn't be greatly missed; but still, this seems like a poor solution (and as more and more fonts become increasingly sophisticated, the problem will just keep recurring in new contexts).

(2) Make the detection of whether a font has features involving <space> (and therefore whether we can use per-word caching) sensitive to the particular OpenType features that are actually enabled. Then we'd only lose caching in the (rare) case where the problem feature is explicitly turned on. However, this would make the detection of those features significantly more complex and expensive.

(3) [My current favorite] Use the CSS text-rendering:optimizeSpeed "hint" as a signal to ignore the presence of features that involve <space>, and allow word caching regardless. Then we can set that property by default for Gaia apps, at least, but if we ever care about actually supporting the thin space within Fira fractions, we can apply text-rendering:optimizeLegibility to the style in question.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: [Gecko part] don't bypass the shaped-word caches because of <space> features if text-rendering:optimizeSpeed is in effect.

This provides the support in Gecko for text-rendering:optimizeSpeed to enable word caching, even if the font has OpenType features that could involve <space>. The other piece needed for this to actually help will be a CSS rule in Gaia or B2G to set the text-rendering property; I'm not sure where is best to do that. Maybe somewhere like gaia/shared/style/headers.css?

Comment 9

[Jonathan Kew [:jfkthame]]

Vivien, what would be the best way to set a CSS property globally for all Gaia apps? To address the issue here, I want to say something like

* { text-rendering:optimizeSpeed; }

Is there a standard place to do that so that everything will inherit it?

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: [b2g part] use text-rendering:optimizeSpeed by default throughout b2g, for better perf with Fira Sans.

Comment 11

[Benoit Girard (:BenWa)]

I'm going to reprofile with this. One moment.

Comment 12

[Jonathan Kew [:jfkthame]]

Cool, it'll be interesting to see what difference it makes. This should not affect the time spent under nsTextFrame::DrawText, but it should reduce the (approx equal amount of) time spent under BuildTextRuns. How much will depend on the repetitiveness of the text runs being created.

It's also possible that we are discarding and re-creating the actual text runs (or text frames) too often; these patches won't affect that aspect of things.

Comment 13

[Benoit Girard (:BenWa)]

Here's the profile:
http://people.mozilla.org/~bgirard/cleopatra/#report=f5d813b20e472d160d006c6929871b5f1f18f4bf&search=nsDisplayText

This profile isn't recorded with the same input so you can't compare them against each other. We still spend a lot of time in nsDisplayText/BuildTextRuns with this patch :(.

Comment 14

[Jonathan Kew [:jfkthame]]

It's noticeable that in this profile, BuildTextRuns is only about 50% of DrawText, whereas in the previous one they were equal. (Roughly.) That makes sense for BuildTextRuns being (on average) faster now that it can use the word cache.

If we're reconstructing text frames (and therefore the text runs they use) too often, however, then we're still going to see a lot of text-run construction overall, even if it's able to use the word cache. John, any insights on the text-run lifetime side of this?

Comment 15

[Kai-Chih Hu [:khu]]

Can we get ETA to fix this bug? Thank you.

Comment 16

[Jonathan Kew [:jfkthame]]

The patch (attachment 8370220 [details] [diff] [review]) awaiting John's review is the only definite "fix" we know of at this point. It should improve things to some extent, though it's not a magic bullet if other aspects of the display are still inefficient.

Comment 17

[John Daggett (:jtd)]

I'm guessing this is more of a problem with too many reflows that
rebuild all text runs. Doing this exacerbates the slowness due to the
fact that the lookups in FiraSans cause the word cache to be bypassed.

The text perf logging will give you a clue about this:

  export NSPR_LOG_MODULES=textperf:5
  export NSPR_LOG_FILE=/path/to/textperf.log

Example output:

(textperf-reflow) 0x11cde6000 time-ms:       0 reflow: 30 chars: 126 content-textruns: 0 chrome-textruns: 3 max-textrun-len: 42 word-cache-lookups: 12 word-cache-hit-ratio: 0.750 word-cache-space: 0 word-cache-long: 0 pref-fallbacks: 0 system-fallbacks: 0 textruns-const: 3 textruns-destr: 3 cumulative-textruns-destr: 177
(textperf-reflow) 0x122fd2000 time-ms:     106 reflow: 1 chars: 33141 content-textruns: 1 chrome-textruns: 0 max-textrun-len: 33141 word-cache-lookups: 6002 word-cache-hit-ratio: 0.427 word-cache-space: 0 word-cache-long: 0 pref-fallbacks: 0 system-fallbacks: 0 textruns-const: 1 textruns-destr: 0 cumulative-textruns-destr: 0
(textperf-reflow) 0x11cde6000 time-ms:       1 reflow: 31 chars: 142 content-textruns: 0 chrome-textruns: 22 max-textrun-len: 43 word-cache-lookups: 38 word-cache-hit-ratio: 0.711 word-cache-space: 0 word-cache-long: 0 pref-fallbacks: 0 system-fallbacks: 0 textruns-const: 22 textruns-destr: 22 cumulative-textruns-destr: 181
(textperf-reflow) 0x122fd2000 time-ms:      93 reflow: 4 chars: 33141 content-textruns: 1 chrome-textruns: 0 max-textrun-len: 33141 word-cache-lookups: 6002 word-cache-hit-ratio: 0.427 word-cache-space: 0 word-cache-long: 0 pref-fallbacks: 0 system-fallbacks: 0 textruns-const: 1 textruns-destr: 1 cumulative-textruns-destr: 0
(textperf-reflow) 0x11cde6000 time-ms:       0 reflow: 33 chars: 126 content-textruns: 0 chrome-textruns: 3 max-textrun-len: 42 word-cache-lookups: 12 word-cache-hit-ratio: 1.000 word-cache-space: 0 word-cache-long: 0 pref-fallbacks: 0 system-fallbacks: 0 textruns-const: 3 textruns-destr: 3 cumulative-textruns-destr: 208
(textperf-reflow) 0x122fd2000 time-ms:     261 reflow: 5 chars: 126542 content-textruns: 1 chrome-textruns: 0 max-textrun-len: 126542 word-cache-lookups: 22759 word-cache-hit-ratio: 0.661 word-cache-space: 0 word-cache-long: 0 pref-fallbacks: 0 system-fallbacks: 0 textruns-const: 1 textruns-destr: 1 cumulative-textruns-destr: 1
(textperf-reflow) 0x122fd2000 time-ms:     906 reflow: 7 chars: 350212 content-textruns: 1 chrome-textruns: 0 max-textrun-len: 350212 word-cache-lookups: 62966 word-cache-hit-ratio: 0.531 word-cache-space: 0 word-cache-long: 0 pref-fallbacks: 0 system-fallbacks: 0 textruns-const: 1 textruns-destr: 1 cumulative-textruns-destr: 2
(textperf-reflow) 0x122fd2000 time-ms:    4434 reflow: 9 chars: 1974887 content-textruns: 1 chrome-textruns: 0 max-textrun-len: 1974887 word-cache-lookups: 354317 word-cache-hit-ratio: 0.525 word-cache-space: 0 word-cache-long: 0 pref-fallbacks: 0 system-fallbacks: 0 textruns-const: 1 textruns-destr: 1 cumulative-textruns-destr: 3
(textperf-loaddone) 0x122fd2000 time-ms:    7307 reflow: 12 chars: 2517923 [http://people.mozilla.org/~jdaggett/tests/largetext-flipcolor.html] content-textruns: 5 chrome-textruns: 0 max-textrun-len: 1974887 word-cache-lookups: 452046 word-cache-hit-ratio: 0.530 word-cache-space: 0 word-cache-long: 0 pref-fallbacks: 0 system-fallbacks: 0 textruns-const: 5 textruns-destr: 4 cumulative-textruns-destr: 4

If you look carefully at the number of textruns built, if a large
number are rebuilt several times, something is causing reflows.  Is
the page tweaking style attributes in script in an onload handler?
Even something like touching body.style.color causes textruns to be
dumped and rebuilt (see bug 950526). Is the page somehow using
downloadable fonts?

Comment 18

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #7)
> Looking into the Fira Sans font (which I assume is what's being used
> here), I can see the reason for this: the font includes an alternate
> <space> glyph that's used when the automatic-fractions feature is
> applied, to provide a "thin space" in sequences like "1 1/2" (when
> rendered as "1½"). That's all very well, but the existence of
> OpenType features that involve <space> disables our word caching (as
> per bug 761442).
>
> What this implies is that throughout FirefoxOS, wherever we're using
> the Fira Sans font, we're losing out on shaped-word caching. That's
> pretty sad.

So this portion of the bug is the same problem as bug 921858, where
some Japanese fonts experienced a slowdown due to space lookups
associated with the hwid feature.

> There are a few possible options we could consider to mitigate this:
> 
> (1) Remove that particular feature from the Fira Sans fonts. It's
> likely to be very rarely used, so it wouldn't be greatly missed; but
> still, this seems like a poor solution (and as more and more fonts
> become increasingly sophisticated, the problem will just keep
> recurring in new contexts).

Right, this is an option but not really what we want to do.

> (2) Make the detection of whether a font has features involving
> <space> (and therefore whether we can use per-word caching)
> sensitive to the particular OpenType features that are actually
> enabled. Then we'd only lose caching in the (rare) case where the
> problem feature is explicitly turned on. However, this would make
> the detection of those features significantly more complex and
> expensive.

==> bug 921858

I don't think the analysis needs to be so complex.  We just need to
*not* bypass the word cache when a font has space lookups only for
non-default features and no font features are specifically enabled.
That's the 99% case I think. I've put a comment about this into bug
921858.

> (3) [My current favorite] Use the CSS text-rendering:optimizeSpeed
> "hint" as a signal to ignore the presence of features that involve
> <space>, and allow word caching regardless. Then we can set that
> property by default for Gaia apps, at least, but if we ever care
> about actually supporting the thin space within Fira fractions, we
> can apply text-rendering:optimizeLegibility to the style in question.

I do not think we should be using an author exposed property like
this, since the problem is really a problem of poor performance
specific to our current implementation.  Fractions should "just work"
when enabled, without any extra twiddling of hints like this. This is
also effectively exposing a feature across all platforms and I don't
think that's a great idea since the utility of this feature will
change over time.

I think it would be better to try and accelerate the fix for bug
921858. I'll try to focus on a patch for that.

Comment 19

[John Daggett (:jtd)]

Comment on attachment 8370220 [details] [diff] [review]
[Gecko part] don't bypass the shaped-word caches because of <space> features if text-rendering:optimizeSpeed is in effect.

As per comment 18, I don't think adding additional behaviors to the
text-rendering property is a good solution here. And it's not clear
from the comments that it actually fixes the problem (comment 13). As
I mentioned in a previous comment, I think the real problem here is
excessive reflows that dump and rebuild textruns.  It would be good to
figure out why textruns are getting dumped as these usually aren't
dumped simply due to scrolling.

Comment 20

[John Daggett (:jtd)]

Comment on attachment 8370282 [details] [diff] [review]
[b2g part] use text-rendering:optimizeSpeed by default throughout b2g, for better perf with Fira Sans.

> * {
>   text-rendering: optimizeSpeed;
> }

Setting aside the merits of adding behavior to text-rendering for a
moment, it's an inheritable property so there's no real reason to use
a universal selector here. Using :root or body would be more
appropriate I think.

Comment 21

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #19)
> Comment on attachment 8370220 [details] [diff] [review]
> [Gecko part] don't bypass the shaped-word caches because of <space> features
> if text-rendering:optimizeSpeed is in effect.
> 
> As per comment 18, I don't think adding additional behaviors to the
> text-rendering property is a good solution here.

ISTM this is very much in line with author expectations as to the kind of thing the text-rendering:optimizeSpeed property might do. (It's documented, for example, as disabling ligatures and kerning; that's not generally true in Gecko these days, but still allowing it to disable certain subtle features that are relatively expensive in our implementation seems entirely reasonable.)

> And it's not clear
> from the comments that it actually fixes the problem (comment 13). 

It's clear that it reduces the overall cost of whatever text-run construction we're doing (comment 14).

> As
> I mentioned in a previous comment, I think the real problem here is
> excessive reflows that dump and rebuild textruns.  It would be good to
> figure out why textruns are getting dumped as these usually aren't
> dumped simply due to scrolling.

If we can do less text-run construction as well, obviously that's also a win. But this is one thing that we could trivially implement right now for b2g 1.3 (my understanding is that they need it immediately); it's not clear we have any other solution ready to deploy.

Comment 22

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #20)
> Comment on attachment 8370282 [details] [diff] [review]
> [b2g part] use text-rendering:optimizeSpeed by default throughout b2g, for
> better perf with Fira Sans.
> 
> > * {
> >   text-rendering: optimizeSpeed;
> > }
> 
> Setting aside the merits of adding behavior to text-rendering for a
> moment, it's an inheritable property so there's no real reason to use
> a universal selector here. Using :root or body would be more
> appropriate I think.

Good point. Consider it changed to :root (in the event we end up using it at all).

Comment 23

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #19)
>  I think the real problem here is
> excessive reflows that dump and rebuild textruns.  It would be good to
> figure out why textruns are getting dumped as these usually aren't
> dumped simply due to scrolling.

It's not clear to me that this is in fact happening. I've tried instrumenting MakeTextRun to log creation of new textruns, and scrolling the MMS list (which I assume is pretty similar to the Contacts list - I couldn't figure out how to easily populate that with a bunch of sample data).

AFAICS, in this case at least, we don't seem to do excessive textrun construction; I see the textruns being created during initial display of the list, but once everything has been rendered once, I can scroll back and forth the entire length of the list (using the "make reference-workload-heavy APP=sms" sample data) without any new textrun creation occurring.

If I pause for a few seconds, then we do start discarding textruns (and frames, maybe?) that are substantially outside the currently-visible area, so if I then scroll again (by more than just a screenful or two), we start needing to re-create them. But this seems pretty reasonable behavior: we're trying to keep the currently-visible and immediately-adjacent content ready to display, while saving memory by discarding content that's further away from the current viewport.

I'm not really familiar with the whole display-list architecture, but is that what's involved here in deciding what data is relevant to keep (currently visible, or nearby) and what should be discarded (further from the visible viewport)? Maybe if memory isn't too tight, we can be less aggressive about discarding things?

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jonathan Kew (:jfkthame) from comment #23) 
> It's not clear to me that this is in fact happening. I've tried
> instrumenting MakeTextRun to log creation of new textruns, and scrolling the
> MMS list (which I assume is pretty similar to the Contacts list - I couldn't
> figure out how to easily populate that with a bunch of sample data).

You can populate contacts app with data by running |make reference-workload-medium| from a gaia clone.This will effectively wipe current data and install reference data for a bunch of apps.

Note, you will have to wait for the contacts database to upgrade after you launch the app the next time.  You will get a white screen for 5 to 10 minutes while this happens.  (There is a bug to speed it up.)

I would not be surprised if contacts is triggering reflows.  We initially build out a skeleton DOM and then incrementally fill in the text details as we scroll.  Its been noted in the past that adding the organization information for a contact is particularly expensive, but we did not know why.

Here is the scroll callback:

https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/views/list.js#L83

Which ultimately populates the DOM here:

https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/views/list.js#L443

I had always thought the slowness here was more due to the number of DOM elements and the use things like descendent selectors in the CSS vs a direct text issue.

What should I have been looking for in the profile to determine things were abnormal?

Thanks for your help!

Comment 25

[Jonathan Kew [:jfkthame]]

Attachment: textrun creation during loading and scrolling in the Contacts app

(In reply to John Daggett (:jtd) from comment #19)
> I think the real problem here is
> excessive reflows that dump and rebuild textruns.  It would be good to
> figure out why textruns are getting dumped as these usually aren't
> dumped simply due to scrolling.

I logged textrun creation that happens during initial loading of the Contacts list, and during scrolling. Similarly to the SMS app, it looks like we're discarding textruns for entries that have been scrolled "significantly" far out of view, perhaps also with a timed-expiration aspect to it. But I don't see evidence here that we're doing a significant amount of redundant reflow and textrun creation during population or scrolling of the list.

Which suggests that unless we can afford the memory to keep scrolled-out-of-view display items hanging around longer, we can't do much to reduce the amount of textrun creation here; but anything we can do to make those textruns cheaper (i.e. making use of the word cache) should help somewhat.

"slowness [...] due to the number of DOM elements and the use things like descendent selectors in the CSS" (comment 24) may well be a more important factor overall, but that's outside the scope of this specific bug, I think.

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to John Daggett (:jtd) from comment #19)
> Comment on attachment 8370220 [details] [diff] [review]
> [Gecko part] don't bypass the shaped-word caches because of <space> features
> if text-rendering:optimizeSpeed is in effect.
> 
> As per comment 18, I don't think adding additional behaviors to the
> text-rendering property is a good solution here. And it's not clear
> from the comments that it actually fixes the problem (comment 13). As
> I mentioned in a previous comment, I think the real problem here is
> excessive reflows that dump and rebuild textruns.  It would be good to
> figure out why textruns are getting dumped as these usually aren't
> dumped simply due to scrolling.

I agree that adding additional magic behaviour to text-rendering is probably not a good idea.  However, the underlying problem definitely exists.  It looks like bug 921858 is a solution (partial? text does contain spaces that won't trigger the extra magic), but for 1.3, I would rather take something like jfkthame's suggestion here.

Can we tie it to something other than the text-rendering property?  Introduce a moz-* hint property?  Or can we just do it unconditionally behind a pref for now?  Any of these things will get us some of the performance back.  It's not a huge amount, but right now we've got a death by a thousand cuts thing going, and this is one of those cuts that we can heal.

Comment 27

[Jet Villegas (inactive)]

As I understand it, for Fira Sans, the only feature we lose is kerning reduction when displaying fraction strings. That seems acceptable here, given the reduced risk we're willing to take for B2G 1.3. For m-c, let's keep a related bug open for a better fix.

Comment 28

[Jonathan Kew [:jfkthame]]

Attachment: [for b2g 1.3] use text-rendering:optimizeSpeed to ensure shaped-word caching is used with Fira Sans.

Given that B2G needs an immediate, minimum-risk fix for 1.3, I think we should go ahead and use this as being the simplest and safest option available. A solution that involves additional analysis of font features/lookups to determine when to disable caching may be fine for the longer term.

Comment 29

[John Daggett (:jtd)]

Benoit, does the attached patch "fix" the problem you were seeing? Or at least enough such that it should worked around via a temporary solution like this?  It would help to have the textperf log for the testcase you're using (see comment 17).

As stated previously, the lack of word caching in this case is bug 921858. I don't think this patch is the right solution to that problem. But *if* this patch ameliorates the problem enough such that we should take a workaround like this, then I would be okay with this as long as it's behind a MOZ_B2G conditional, with the understanding that the fix for bug 921858 will remove it entirely.

I guess the fundamental question I still have here is - does enabling word caching fix the underlying sluggishness that's considered critical to fix for B2G 1.3?

Comment 30

[Benoit Girard (:BenWa)]

Is this patch different then attachment 8370282 [details] [diff] [review]? There I didn't see a significant improvement when I tested. Perhaps precise timing may report an improvement but we're looking to remove the bottleneck and I don't think it's sufficient alone.

Comment 31

[John Daggett (:jtd)]

(In reply to Benoit Girard (:BenWa) from comment #30)
> Is this patch different then attachment 8370282 [details] [diff] [review]?
> There I didn't see a significant improvement when I tested. Perhaps precise
> timing may report an improvement but we're looking to remove the bottleneck
> and I don't think it's sufficient alone.

Thanks Benoit. The new patch is simply a consolidation of the two original patches (attachments 8370282, 8370220).  Just to confirm, you tested with both of these patches applied?

Comment 32

[John Daggett (:jtd)]

Also, what was the size of the contact list you were using when running your tests? And does the amount of time spent in rendering the contact list lead to a noticable sluggishness? Or is this more in a "we shouldn't be doing this" category?

Comment 33

[Milan Sreckovic [:milan] (needinfo for best results)]

Given this is not giving us the improvements at the level they would overcome the risk of making a change in beta, removing 1.3+.  We should land it on the current trunk though.

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

I'm testing attachment 8372192 [details] [diff] [review] on v1.3t as I have been seeing text run creation show up in stacks during visible pauses on the tarako device.

Comment 35

[John Daggett (:jtd)]

Comment on attachment 8372192 [details] [diff] [review]
[for b2g 1.3] use text-rendering:optimizeSpeed to ensure shaped-word caching is used with Fira Sans.

Given this is no longer required for beta and that I should have a tested patch ready soon on bug 921858, I'm going to r- this for now.

Comment 36

[Benoit Girard (:BenWa)]

(In reply to John Daggett (:jtd) from comment #32)
> Also, what was the size of the contact list you were using when running your
> tests? And does the amount of time spent in rendering the contact list lead
> to a noticable sluggishness? Or is this more in a "we shouldn't be doing
> this" category?

I was using 500 contacts. But the visibility culling was removing offscreen nodes and adding text nodes for newly exposed content. This bug (and the app JS time) we're both bottlenecking painting and leading to multi-second checkerboard. So in the 'real problem with bad user impact' category.

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

This appears to be heavily influenced by the change in bug 942460.  When I backed that out I saw nsDisplayText time drop significantly while scrolling on contacts.  For example, the largest block of nsDisplayText dropped from 300ms to 50ms.

Here is some more info originally posted in bug 967884 comment 62:

Profile and video of current v1.3:

  http://people.mozilla.org/~bgirard/cleopatra/#report=0d9508e50a780928fda3aba64ae12e580d58e726
  https://www.dropbox.com/s/ek6ae6ey0i2mupq/20140219_contacts_v13_sticky.MOV

Profile and video of v1.3 with bug 942460 backed out:

  http://people.mozilla.org/~bgirard/cleopatra/#report=b66ffcf5ebaa76e4107a97485dae9d7343c14ff8
  https://www.dropbox.com/s/njcgfvx0fwudubi/20140219_contacts_v13_no_sticky.MOV

Comment 38

[John Daggett (:jtd)]

Doing some profiling of the Contacts with latest trunk code, I don't see a huge percentage of time spent in textrun construction. Depending upon the exact testing steps, there's a variety of ratios spent in appcode/layout/paint.

Setup: local B2G build with gaia "heavy" reference load, testing on Peak device

Profile #1 - Contacts app startup

https://people.mozilla.org/~bgirard/cleopatra/#report=8257b6ad6b65495954949c40bf7d6ccace9c98e4

Steps:

1. enable the profiler on b2g and the "(Preallocated..."
2. start up the Contacts app
3. try scrolling, multi-second delay until one can scroll
4. after scrolling starts, scroll for a few more seconds
5. capture profiling data

Percentage times:

54.9 Startup::XRE_InitChildProcess
     38.2 nsTimerEvent::Run
          19.8 nsViewManager::ProcessPendingUpdates ==> paint
          15.4 layout::Flush ==> layout
           2.1 cycle collector
      4.9 nsOutputStreamReadyEvent ==> read from JAR, ...?
16.5 ContactManager.prototype._fireSuccessOrDone
10.7 ContactManager.prototype._convertContacts
 9.2 Timer::Fire ==> paint 6.2 layout flush 2.2
 5.3 scrollHandler() @ tag_visibility_monitor.js:150 ==> layout flush

The code in scrollHandler is accessing container.scrollTop, which
causes a synchronous layout flush, which in some cases could cause jank:

http://mxr.mozilla.org/gaia/source/shared/js/tag_visibility_monitor.js#150

Profile #2 - Contacts app scrolling the index

https://people.mozilla.org/~bgirard/cleopatra/#report=1fe42bc20df5ba1dbf8f2d6f7ea34de029a31f9c

1. enable the profiler on b2g and the "(Preallocated..."
2. start up the Contacts app
3. wait 15 seconds
4. touch and scroll down the right-side index ("A B C ..."), down and up, twice
5. capture profiling data

Almost all the time spent within Paint functions.

In some cases there seems to be pathological nesting in place. Within  Paint::PresShell::Paint, a call to nsImageRenderer::PrepareImage is nested within calls to nsBlockFrame::BuildDisplayList 11 levels deep.

Profile #3 - Contacts app scrolling from top to bottom

https://people.mozilla.org/~bgirard/cleopatra/#report=b83c151430bbe2447dc5ed9a4ed11aa99bfd6918

1. enable the profiler on b2g and the "(Preallocated..."
2. start up the Contacts app
3. wait 15 seconds
4. simply scroll down the entire list of contacts *without* using the index
5. capture profiling data

Here again, most of the time is spent in paint operations.

This app also appears to have some strange perf characteristics. Scrolling down the list and stopping at a name, then selecting a contact and hitting back leads to poorer scrolling performance.  While scrolling seemed okay before looking at the contact, after looking at it the screen often just shows white.

At this point, I'm thinking that it would make sense to close this bug. To me, it seems like the Contacts app has some fundamental design issues that adversely affect performance.  But this bug really isn't the place to tackle those.  The lack of a word caching when Fira Sans is used will be dealt with in bug 921858.

Comment 39

[Mike Lee [:mlee]]

Ben,

Do you agree with John's proposal in comment 38 to close this bug? If so please close this out, otherwise please comment on what approach you think makes sense moving forward.

Thanks,
Mike

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

This no longer seems to be an issue.  I believe tiling has effectively mitigated the issue by breaking up our large paints.  Its possible this may still effect 1.3, though.