981942 - setting contact selection checkbox state triggers unnecessary reflows while scrolling

Status: RESOLVED INVALID

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

Description

[Ben Kelly [:bkelly, not reviewing]]

With John Daggett's help this evening I was able to identify the source of some of the extra reflows in contacts scrolling.  Specifically, why we are regenerating text runs while scrolling contacts that have already been rendered.

It appears to be this code in list.js:

  var updateSingleRowSelection = function updateSingleRowSelection(row, id) {
    var id = id || row.dataset.uuid;
    var check = row.querySelector('input[value="' + id + '"]');
    if (!check) {
      return;
    }

    check.checked = !!selectedContacts[id];
  };

This unconditionally sets the value of the checkbox to the backing selection state of the contact model.  Most of the time this will be setting the checkbox to the same value it already contains.

Unfortunately this seems to trigger a reflow and the regeneration of a bunch of textruns.

Talking to Daniel Holbert this appears to be a platform bug.  I'm going to propose a gaia work around here and then open another bug for the platform issue.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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

This avoids the additional reflows when scrolling content that is already rendered.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Profile of contacts after the change:

  http://people.mozilla.org/~bgirard/cleopatra/#report=86e9331c1bddd2b30df35f26e4953411f9fbd639

Profile before the change, but I'm not sure I got enough history:

  http://people.mozilla.org/~bgirard/cleopatra/#report=33b715fb04933dae2e5f39cc4d38810e1a991244

Comment 3

[John Daggett (:jtd)]

Looks good, there's a lot fewer reflows showing up in the newer profile.

Comment 4

[Francisco Jordano [:arcturus] [:francisco]]

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

LGTM, thanks for catching that one!

Do you think we could add unit tests to the PR?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I think I could add some asserts to the existing unit tests to validate the checked attribute is set correctly.  I don't think I can validate the (lack of) reflows very easily, though.

Comment 6

[Francisco Jordano [:arcturus] [:francisco]]

Will be more than enough just checking that we don't change the checked attribute.

By the way, not for doing it here, but just found a place where Etienne is using integration tests to detect that:

https://github.com/etiennesegonzac/gaia/blob/e5248fadbc2fbb58fc4e00b1a35b86adbf0062c8/tests/js-marionette/reflow_helper.js

https://github.com/etiennesegonzac/gaia/blob/e5248fadbc2fbb58fc4e00b1a35b86adbf0062c8/apps/communications/dialer/test/marionette/keypad_test.js

Just to remind my self for future tests :)

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Hmm, it appears changing the checked assignment may have been a false positive.  If I keep scrolling I eventually see the re-created textruns again.

For example:

1) make reference-workload-medium
2) scroll to bottom of the list to get all items rendered... lots of textruns while we scroll
3) scroll back to start of list... about half way textruns start getting created again

With reflow logging enabled I don't see any reflows.  I also don't see any textperf-reflow NSPR log messages.  Just the textrun logs.

So the checkbox thing is probably a red herring.

John, should I expect the textruns to get re-created in this case?

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

It appears text runs get expired every 10 seconds in FrameTextRunCache.  So this is normal and expected.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Closing this as invalid.

Note, I think overall we are in better shape with contacts painting text now that tiling has landed.  We no longer need to repaint huge portions of the list, and remaking their text runs, in one go.

Comment 10

[John Daggett (:jtd)]

(In reply to Ben Kelly [:bkelly] from comment #7)
> Hmm, it appears changing the checked assignment may have been a false
> positive.  If I keep scrolling I eventually see the re-created textruns
> again.

That's true but are you sure the change in state doesn't cause textruns to be dumped. I guess the question is if you scroll for less than 10 seconds with and without the patch here, do you see the same number of textruns built or less after the patch?

Comment 11

[John Daggett (:jtd)]

(In reply to Ben Kelly [:bkelly] from comment #7)
> Hmm, it appears changing the checked assignment may have been a false
> positive.  If I keep scrolling I eventually see the re-created textruns
> again.
> 
> For example:
> 
> 1) make reference-workload-medium
> 2) scroll to bottom of the list to get all items rendered... lots of
> textruns while we scroll
> 3) scroll back to start of list... about half way textruns start getting
> created again
> 
> With reflow logging enabled I don't see any reflows.  I also don't see any
> textperf-reflow NSPR log messages.  Just the textrun logs.
> 
> So the checkbox thing is probably a red herring.
> 
> John, should I expect the textruns to get re-created in this case?

Yes, because textruns might be rebuilt during the paint cycle rather than because of a reflow. The comparison should be the number of textruns before/after the checkbox change, assuming you can test in an equivalent manner.