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.
I should note that fixRange() is called for every mutation of the DOM being monitored...
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.
Any thoughts on this, Evan?
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.
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%.
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!
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.
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.
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.
Green travis except for a known, unrelated issue in the ui tests. Landed: https://github.com/mozilla-b2g/gaia/commit/ea967f79e5f29e752b9abb4df14fc334946560a0