921808 - unnecessary style changes trigger reflow/paint during load

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect, P1)

Description

[Ben Kelly [:bkelly, not reviewing]]

As can be seen in the following profile at time 17742 there is an additional paint after the list is initialized:

  http://people.mozilla.org/~bgirard/cleopatra/#report=1f2278ec0bed9920679b4e1d93c786991a5f26be

Part of this is due to the dynamic group list creation (bug 921683), but some of it is also caused by the checkCancelableActivity() function in contacts.js.

This function effectively does this:

      cancelButton.classList.add('hide');
      addButton.classList.remove('hide');
      settingsButton.classList.remove('hide');
      appTitleElement.textContent = _('contacts');

These writes to the DOM are unnecessary in this case because the static index.html already specifies these classes and text content.

It would be nice if gecko recognized that no change has occurred, but unfortunately it does not.  For example, see bug 845205 where removing a non-existent style triggers a reflow.

Therefore, we can avoid about 25ms of reflow/paint work if we simply check the state of the classList and textContent before making the changes.

Pull request to come.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Pull request at https://github.com/mozilla-b2g/gaia/pull/12527

This pull request adds some helper routines to check the class state before setting/removing the class.  Annoying, but effective in this case.  The patch also checks the textContent as well before setting.

Profile before at time 17742:

  http://people.mozilla.org/~bgirard/cleopatra/#report=1f2278ec0bed9920679b4e1d93c786991a5f26be

Profile after at time 13357:

  http://people.mozilla.org/~bgirard/cleopatra/#report=1ce807a301d8cfbb72562d7e9004f95bdd2ff3c4

These show the reflow/paint is reduced from 50ms to 25ms.  I hope to remove this reflow/paint event completely once bug 921683 is implemented.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Nom koi? because it blocks a koi+.

Comment 3

[Jose Manuel Cantera]

Comment on attachment 811632 [details]
Pull request at https://github.com/mozilla-b2g/gaia/pull/12527

good work Ben.

thanks

PS: Before landing, please check unit tests are Travis seem to be complaining

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Thanks for the review!  The tests were an unrelated intermittent failure in email.

I'm going to hold off on landing for a little bit to see if I can fix this in gecko.  It *looks* like it might be an easy fix, but you never know with the style system.  If it becomes too complicated or triggers mysterious tbpl errors I will proceed with this patch.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

So I spent a day or so investigating if the classList part of this patch could be fixed in gecko in bug 922677.  The conclusion I came away from that is that gecko already adequately handles no-op classList add()/remove().  My short-circuit gave some improvement on a micro-benchmark, but that was simply due to avoiding code which is necessary to fire mutation handlers if present.  Since the spec requires mutation handlers to fire in that case we can't use the short-circuit.  In any case, it wasn't causing a paint.

So I looked closer at the textContent assignment.  I now believe this is the only part of this code that is causing a repaint here.  Since the profile has so much going on I decided to verify my belief about the paint a second way.

First, I disabled the hwc by commenting out the block here as suggested by :BenWa:

  http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#322

With the hwc disabled I could then use the "paint flashing" pref in the settings app.  This shows that the the 'Contacts' title is repainted when textContent is re-assigned with the same value.

At a minimum I'm going to rework the pull request to only protect the textContent assignment.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

:bz points out that fixing the textContent assignment repaint in gecko would be difficult and has been discussed before.  See bug 725221.

I'll proceed with the app side patch given current priorities.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Since the revised patch was essentially the same as previously reviewed I carried for the r+.

Merged:

  https://github.com/mozilla-b2g/gaia/commit/24234de42e9418e5cc602133f482dee7524b887f

Comment 8

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 24234de42e9418e5cc602133f482dee7524b887f to:
v1.2: 44fd89ee80a16c1416c72bc83ed08b12340d87d5