unnecessary style changes trigger reflow/paint during load

RESOLVED FIXED in Firefox OS v1.2

Status

Firefox OS
Gaia::Contacts
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Depends on: 1 bug, {perf})

unspecified
1.2 C2(Oct11)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, b2g-v1.2 fixed)

Details

(Whiteboard: [c= p=1 s=2013.10.04 u=1.2])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 811632 [details]
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.
Attachment #811632 - Flags: review?(jmcf)
(Assignee)

Updated

5 years ago
Attachment #811632 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 2

5 years ago
Nom koi? because it blocks a koi+.
blocking-b2g: --- → koi?

Comment 3

5 years ago
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
Attachment #811632 - Flags: review?(jmcf) → review+
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 922677
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
: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.
(Assignee)

Updated

5 years ago
Depends on: 725221
(Assignee)

Comment 7

5 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
blocking-b2g: koi? → koi+
Priority: -- → P1
Whiteboard: [c= p=1 s= u=] → [c= p=1 s=2013.10.04 u=1.2]
Uplifted 24234de42e9418e5cc602133f482dee7524b887f to:
v1.2: 44fd89ee80a16c1416c72bc83ed08b12340d87d5
status-b2g-v1.2: --- → fixed

Updated

5 years ago
Target Milestone: --- → 1.2 C2(Oct11)
You need to log in before you can comment on or make changes to this bug.