tag_visibility_monitor triggers too many reflows

RESOLVED FIXED in Firefox OS v1.2

Status

Firefox OS
Gaia
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks: 1 bug, {perf})

unspecified
1.2 C3(Oct25)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

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

Attachments

(2 attachments)

(Assignee)

Description

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

Comment 1

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

Comment 2

4 years ago
Created attachment 807036 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Blocks: 865747

Updated

4 years ago
Whiteboard: [c= p= s= u=]
(Assignee)

Updated

4 years ago
blocking-b2g: --- → koi?
blocking-b2g: koi? → koi+

Updated

4 years ago
Priority: -- → P1
Whiteboard: [c= p= s= u=] → [c= p= s= u=1.2]
(Assignee)

Updated

4 years ago
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Any thoughts on this, Evan?
Flags: needinfo?(eshapiro)
(Assignee)

Updated

4 years ago
Blocks: 915118
(Assignee)

Updated

4 years ago
Blocks: 922316
(Assignee)

Comment 4

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

Updated

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

Comment 5

4 years ago
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%.
Target Milestone: --- → 1.2 C3(Oct25)
(Assignee)

Updated

4 years ago
No longer blocks: 915118
(Assignee)

Updated

4 years ago
Whiteboard: [c= p= s= u=1.2] → [c= p=4 s= u=1.2]
(Assignee)

Comment 6

4 years ago
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!
Flags: needinfo?(dflanagan)

Comment 7

4 years ago
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.
Flags: needinfo?(eshapiro)
(Assignee)

Comment 8

4 years ago
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 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.
Attachment #816810 - Flags: review?(dflanagan) → review+
(Assignee)

Comment 10

4 years ago
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.
Flags: needinfo?(dflanagan)
(Assignee)

Comment 11

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

  https://github.com/mozilla-b2g/gaia/commit/ea967f79e5f29e752b9abb4df14fc334946560a0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

4 years ago
Uplift to v1.2:

  https://github.com/mozilla-b2g/gaia/commit/b67568c07ccde602b43115efade3170234ac95a5
status-b2g-v1.2: --- → fixed
You need to log in before you can comment on or make changes to this bug.