918179 - tag_visibility_monitor triggers too many reflows

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

While profile bug 914913 I noticed a lot of time was being spent in tag_visibility_monitor's fixRange() function.  It appeared that every time fixRange() was called gecko was forcing a reflow.

Further, it looked like this was ultimately due to relativeVisibilityPosition() touching DOM properties like scrollTop, offsetTop, etc.

I tried modifying fixRange() to only trigger a recomputation if necessary.  It did reduce the work performed, but caused some of the tests to fail.

This could use additional investigation.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I should note that fixRange() is called for every mutation of the DOM being monitored...

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: tag_vm_opt.patch

This is the naive patch I tried.  It did help avoid reflows, but broke the tests.  Obviously we need a different conditional here.

Comment 3

[David Flanagan [:djf]]

Any thoughts on this, Evan?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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

I was able to at least optimize the case needed by contacts.  This avoids the sync reflows when appending elements offscreen.

I'm concerned, however, that there is a moderately sized foot gun here.  If we can't handle the general case efficiently, perhaps we should add more assumptions, trim functionality, and make the implementation simpler.  For example, adding the restriction that elements cannot be removed would simplify the mutation handler logic.  Or perhaps we could try some alternative implementation ideas.

Thats probably a topic for a separate bug, though.  I'd like to at least land this initial fix for v1.2.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Oh, I also profiled to verify the sync reflows are gone:

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

For comparison, this is an earlier profile:

  http://people.mozilla.org/~bgirard/cleopatra/#report=7b4c5b7c440fa13c39d4dab0464d9679272eacda

The mutationHandler as percent of total app load time drops from 12% to 1%.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

David, should I investigate having Vivien or someone take the review on this?  I know you're buried in work, but I was hoping to get it committed/uplifted by the end of this week (10/25).  Thanks!

Comment 7

[eshapiro]

I took a look at the pr, one of the variable names don't quite make sense to me, but besides that the changes look good.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Thanks Evan!  I updated the pull request with the new variable name you suggested.  I also expanded the comments a bit and rebased to latest master.

Comment 9

[David Flanagan [:djf]]

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

I left some suggestions on github, but this code looks good to me. The fact that the author (Evan) has looked at it increases my confidence.

I don't know how to detect reflows, but I'm assumining that you've tested, Ben, and that this patch solves your problems.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Thanks David!  I've made the suggested changes and pushed an update to the github branch.  I added comments indicating that the sync reflows stem from touching scrollTop and offsetTop in the relativeVisibilityPosition() function.

And yes, I've verified using the profiler that the sync reflows no longer occur in contacts with this patch.  Of course, if the DOM is modified by removing a node or inserting something earlier in the structure then a sync reflow will occur.  I'm not sure how to avoid this, though, given the guarantees of the visibility monitor API to notify about visibility changes when a mutation occurs.

I'm just waiting for travis to run and then I will land this.  Thanks again.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Green travis except for a known, unrelated issue in the ui tests.  Landed:

  https://github.com/mozilla-b2g/gaia/commit/ea967f79e5f29e752b9abb4df14fc334946560a0

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Uplift to v1.2:

  https://github.com/mozilla-b2g/gaia/commit/b67568c07ccde602b43115efade3170234ac95a5