feat: Use fastdom to batch writes and avoid sync layout #385

STRML · 2018-07-30T16:41:45Z

When animating a large number of elements, it can be helpful to batch these animations up into a single mutate operation using fastdom to improve forced synchronous layout stalls.

jquense · 2018-07-30T16:47:11Z

src/utils/ClassHelpers.js

+    if (process.browser) {
+      // eslint-disable-next-line global-require
+      if (!visibilityWatcher) visibilityWatcher = require('visibility')();
+      if (visibilityWatcher.visible()) {


I think !document.hidden is probably fine instead?

Yeah, old habits die hard; updated

jquense · 2018-07-30T16:49:04Z

src/utils/ClassHelpers.js

+      // eslint-disable-next-line global-require
+      if (!visibilityWatcher) visibilityWatcher = require('visibility')();
+      if (visibilityWatcher.visible()) {
+        fastdom.mutate(run);


What is this doing exactly? It feels like overkill if all that's happening is it's its running tasks in a RAF loop no?

It also batches them into a single transaction (see https://github.com/wilsonpage/fastdom#how-it-works) which can avoid thrashing.

However, if you don't use the visibilityWatcher, a bunch of rAF calls will batch up and cause a lag while you switch back to the tab, as all those batched up calls execute at once.

jquense · 2018-07-30T16:49:54Z

src/CSSTransition.js

-    // which is necessary in order to transition styles when adding a class name.
-    if (className) {
-      /* eslint-disable no-unused-expressions */
-      node && node.scrollTop;


Why is the reflow being removed?

The point is to avoid synchronous layout, which this forces. In my testing it was no longer necessary when using fastdom as the class manipulation happens on another tick.

I'm a bit concerned about this, bc my previous testing trying to use rAF directly instead of the sync reflow led to animations not running. I wish I had a specific test case to try tho, I don't honestly remember what the cases were. Maybe during interrupts or fast toggling?

I haven't been able to replicate any problems in my testing - ideas?

theoretically, what is ensuring that active classes are being added on a different tick than the initial classes? From what I can tell here, every call to fastdom.mutate adds to the same queue, if the initial and active classes are added within the the rAF time frame (very likely?) they will flush on the same tick.

To follow up on this, we've been using this in prod for 3mo+ without any issue. The forced reflow is unnecessary as fastdom will delay the change until the next animation frame.

The -active classes work as expected, I tested this by adding opacity: 0 to -enter and opacity: 1 to -enter-active. The element faded in, which wouldn't happen if they were added in the same tick.

olehreznichenko · 2018-10-07T14:29:26Z

Any progress in this PR?
node.scrollTop triggers a lot of force reflows in my project :(

STRML · 2018-10-07T14:47:24Z

We're using this in prod with no issue. I'll resolve the conflict but it's up to the maintainers to decide if they want this.

jquense · 2018-10-08T12:54:02Z

@silvenon i'd appreciate your thoughts here.

I think i'm fine with this generally. My only concern is that fastdom is not a widely used package, and for most folks this is going to be just dead weight. I really do like the option tho to have better control over this, handling reflows and transitions across pages with multiple components sucks and this seems like it'd make an improvement.

silvenon

I was manually stress-testing this and I didn't find any regressions. 👍 I didn't test performance because that goes beyond my frontend skills, but I trust that it worked well in your project.

As far as I'm concerned, this works well. Thanks!

silvenon

Actually, I was linking the package wrong when I was testing it, so I was testing the latest release. 🤦‍♂️ The -active classes aren't being added on next tick, so this changes the important part of the behavior.

silvenon · 2018-10-08T16:59:41Z

To try this for yourself, use the following code:

<CSSTransition appear in timeout={300} classNames="item">
  <div>Item</div>
</CSSTransition>

.item-appear {
  opacity: 0.01;
}
.item-appear-active {
  opacity: 1;
  transition: opacity 300ms;
}

You'll see that the item doesn't fade in, it just instantly appears.

You probably didn't need the appearing functionality, or maybe you were using it in a way which causes a delay which you didn't mind. But adding -active classes on next tick is crucial for appear to be timed correctly.

STRML · 2018-10-09T13:15:31Z

Ok. I'm out for a little while for some family events so I won't be able to get to this immediately but I'll add it to my list to review so I can ensure existing behavior is maintained.

Re: fastdom package weight, it's not zero, but it's very small (46kB full package in node_modules, 1.2kB minified, 626B min+gz).

taion · 2018-10-09T13:28:34Z

The "transaction" thing is just... running them all synchronously, no? https://github.com/wilsonpage/fastdom/blob/v1.0.6/fastdom.js#L161-L207

I wonder if we can be smarter and do our own batching given that we have the whole transition group/transition setup.

@STRML, in your case, are your transitions all nested under a group component?

STRML · 2018-10-09T13:33:13Z

You could do your own batching. For what it's worth, I use fastdom manually in other parts of our codebase so it was very convenient to reuse it here.

All fastdom essentially does is batch "reads" and "writes" and flush them synchronously at rAF, reordering the reads before the writes so you don't end up with layout thrashing.

For what it's worth, it's very well tested and 600 bytes' savings is not really enough incentive IMO to make it worth reimplementing, considering the test overhead and probability of bugs.

taion · 2018-10-09T13:34:01Z

Sure – it's not about the space savings here. I'm wondering more whether we can do a better job at it, given that we have a semantic concept of transition groups/&c.

STRML · 2018-10-09T13:46:20Z

Yeah sure that makes sense. I won't have time to implement it but I'm happy to test and review.

…

On Tue, Oct 9, 2018, 7:19 PM Jimmy Jia ***@***.***> wrote: Sure – it's not about the space savings here. I'm wondering more whether we can do a better job at it, given that we have a semantic concept of transition groups/&c. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#385 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABJFP47s8nHOyf8hdR1r1cY1fH_61D1Bks5ujKXMgaJpZM4Vmw3r> .

STRML · 2019-01-14T14:54:31Z

Hi - was there something else this PR needed?

…

On Mon, Jan 14, 2019, 8:52 AM Jason Quense ***@***.*** wrote: Closed #385 <#385>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#385 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABJFP98URKgrsuDoaPjabsDKkZ0ezHL_ks5vDJnEgaJpZM4Vmw3r> .

jquense · 2019-01-14T14:58:10Z

sorry, I just saw the changed requested and assumed it was stale

jquense · 2019-01-14T14:58:39Z

@silvenon if you're concerns are addressed can you review and we can get this moving along

diagramatics · 2019-04-26T04:38:00Z

I hate to be that person but I'm prodding again to see if we can get the ball rolling — would love to see some performance improvements in react-transition-group hitting the next version.

I've made #496 to fix the enter/appear animations not playing properly and fixing the merge conflict. Explanation on the fix and the results are in the PR. I was about to hijack this PR and push a fix to the branch but I can't since it's in another repo, so I had to make a new PR. Sorry!

STRML · 2019-04-26T16:59:16Z

Rebased on master and incorporated @diagramatics fix for #496. Thanks!

STRML · 2019-04-26T18:41:24Z

I'm actually still seeing issues in storybook with this, in particular the "animates on appear" story is not working right. If I force a reflow in the fastdom.mutate() call it fixes it, but then fastdom is doing effectively nothing, because we're alternating reads and writes and thrashing the dom. Needs further investigation.

diagramatics · 2019-04-29T08:42:39Z

Okay, I looked at it again and I figured might as well keep the reflow logic in, but batch them too with fastdom.measure. I feel like most of our performance issues are around the constant read/write of DOM styles on multiple elements as opposed to the writes not being batched (the browser batches them into one single layout if we don't invalidate them in between writes AFAIK). Since now we batch the invalidation and writes to just one each instead of every one of them, I'm pretty sure we'll still see a similar improvement.

Here's a patch to the current PR @STRML:

diff --git a/src/CSSTransition.js b/src/CSSTransition.js
index 29837a6..91161b7 100644
--- a/src/CSSTransition.js
+++ b/src/CSSTransition.js
@@ -3,7 +3,7 @@ import React from 'react';
 
 import Transition from './Transition';
 import { classNamesShape } from './utils/PropTypes';
-import { addClass, removeClass } from './utils/ClassHelpers';
+import { addClass, removeClass, triggerReflow } from './utils/ClassHelpers';
 
 /**
  * A transition component inspired by the excellent
@@ -85,6 +85,7 @@ class CSSTransition extends React.Component {
       appearing ? 'appear' : 'enter'
     );
 
+    triggerReflow(node)
     addClass(node, activeClassName)
 
     if (this.props.onEntering) {
@@ -122,6 +123,7 @@ class CSSTransition extends React.Component {
   onExiting = (node) => {
     const { activeClassName } = this.getClassNames('exit')
 
+    triggerReflow(node)
     addClass(node, activeClassName)
 
     if (this.props.onExiting) {
diff --git a/src/utils/ClassHelpers.js b/src/utils/ClassHelpers.js
index c039b2d..4e0dcf9 100644
--- a/src/utils/ClassHelpers.js
+++ b/src/utils/ClassHelpers.js
@@ -9,6 +9,10 @@ export function removeClass(node, classes, immediate) {
   mutateClass(node, classes, immediate, removeOneClass);
 }
 
+export function triggerReflow(node) {
+  fastdom.measure(() => node.scrollTop);
+}
+
 function mutateClass(node, classes, immediate, fn) {
   if (!node) return;
   if (classes && typeof classes === 'string') {

WIthout this patch, try the storybook; you'll see a class of 'undefined' being added to elements on mount because the 'appearClassName' and 'enterClassName' are undefined.

STRML · 2019-04-29T16:01:08Z

Hey @diagramatics. Unfortunately that doesn't actually do it, as you can see in the storybook.

Unfortunately I don't think we can actually 100% get what we want, to either batch all reflows (reads) in a single operation or all className edits (writes) in a single operation, without a significant refactor of this module.

For example, on an entering widget, we would have to set up operations in this order:

Remove exit classes
Reflow
Add enter classes

This is a write, a read, then a write, and if we put this through fastdom it's going to reorder it and the animation won't show properly.

I can see a possible optimization where we define batch types (appear/enter, reflow, exit/leave), and process those in batches. This would require maintaining a state machine per element to determine which operation it needs, then schedule reads and writes as necessary. I don't know if there is enough benefit to doing this versus this PR, which at least avoids doing a forced synchronous reflow by scheduling on rAF, and does 50% fewer DOM reads by only reflowing on the initial class add.

diagramatics · 2019-04-30T02:51:02Z

Sorry, not sure I'm following where the issue lies. I think I'm missing some extra info. Let me try and understand your comments one by one.

Unfortunately that doesn't actually do it, as you can see in the storybook.

Which story in particular? I tested all of them (with my change and also with your change) and both work fine in the CSS Transition Group category.

For example, on an entering widget, we would have to set up operations in this order:

Remove exit classes

Reflow

Add enter classes

Do you have an example of this in action? I can't think of any that would add an exit class, then an enter class. I always thought exiting and entering actions happen separately — if you want to exit, then exit -> exiting -> exited and then by the time exited happens you'll get a relayout naturally from the browser since we don't process the transition between exited to enter apart from just triggering a callback to start the enter -> entering -> entered process.

I can see a possible optimization where we define batch types (appear/enter, reflow, exit/leave), and process those in batches. This would require maintaining a state machine per element to determine which operation it needs, then schedule reads and writes as necessary.

I thought we can leverate fastdom for this? Since it does have mutate and measure which means we can just batch reflow calls to measure and class mutations to mutate.

I feel like your transition uses are way more cooler than mine, which is just a simple fade in/out on multiple images at once 😅 and I think that's why I don't fully grasp the issue you have. My issue is around the fact that when you try and do that, it doesn't batch the force reflows as #496 screenshots highlight. I think triggering reflows before the entering/exiting state is inevitable (enter -> force reflow to entering -> entering -> browser reflow -> entered) since we do need the browser to understand and commit the changes in the enter state first to transition from, but with fastdom at least we get the batching capabilities when we want to animate multiple elements. What do you think?

diagramatics · 2019-04-30T02:53:02Z

src/utils/ClassHelpers.js

+      // need to be done on every single class change, only when the 'active' class is added.
+      if (classes.indexOf('active') !== -1) {
+        forceReflow(node);
+      }


I'm actually against doing this here since it makes the class helpers seem very specific to that active prefix being there. I think you can change the prefix or set custom CSS classes to apply for?

Doing it in onEntering and onExiting just like the previous reflowAndAddClass function usage seems like the safer option to me. Plus since it's the only place where the active classes are going to be added I don't think we're going to miss anything. Plus it's safer when the logic flow is pretty much similar, so we don't end up causing breaking changes to anyone else.

Also in addition, why not wrap it in a fastdom.measure?

You're right about the active bit, I've fixed in the latest commit.

In my testing it's only needed on onEntering.

Using fastdom.measure() does the reflow in a different tick and doesn't show the animation.

STRML · 2019-04-30T02:58:27Z

With your patch, I don't see either enter or appear animations working in Storybook's "Css Transition Group -> Animates on all" or "Animates on enter" or "Animates on appear".

Regarding the reflow bit, I'm not explaining it well. The simplest explanation is that you need a reflow between adding "enter" and adding "enter-active", same with "appear" and "appear-active". Without it, the animation won't trigger.

diagramatics · 2019-04-30T03:04:47Z

Sounds about right! I thought my change covers that reflow by queueing it in fastdom.measure before fastdom.mutate occurs.

Odd that my patch doesn't work on your end. Could it be browser? I haven't tested it in other places other than Chrome, I have to admit 😅

STRML · 2019-04-30T03:06:12Z

I'm using Chrome to test it as well, Chrome 74. However if you put the reflow in measure, you'll end up with reflow -> 'enter' -> 'enter-active' when it needs to be 'enter' -> reflow -> 'enter-active'

diagramatics · 2019-04-30T05:13:21Z

That's very weird. I thought since fastdom batches measure and mutate in the next animation frame, the event loop that adds the enter classes would surely queue a style recalculation for an appearing/disappearing element. If it doesn't then measure would surely trigger a reflow in that animation frame before the mutation.

That's when I test the appear story. Does your timeline look like that?

Otherwise I'm generally happy with how you've done it because that's what I thought fastdom would do (measure and reflow before mutate). The issue with not using fastdom.measure is that you lose the batching, so we're back to square one: multiple layout thrashing, just on the next animation frame instead 😞

jquense · 2019-04-30T12:23:19Z

its a super thorny area folks :/ that really steams from the core problem that css transitions really aren't meant to be triggered programmatically in this way. The reflow stuff vaguely defies logic, with requestAnimationFrame's not working and you'd expect and hard sync reflows between frames working great. The other added rub is that trying to do this via React's state model means we are actively dealing with two schedulers for updates and react can "fall behind" or flush at seemingly odd moments some times.

as with a lot of platform primitives, when you need to do any serious animation work in the context of an app, i tend to think you almost always need to use a js framework like velocity or GSAP (both of which work nicely on top of Transition btw). Apart from that it always feels like this bespoke cat wrangling of animations. That said i'm Really not an expert and if ya'll are happy to dig in or propose even drastic refactors i'm more than open to it

diagramatics · 2019-05-01T02:46:44Z

I'm happy to leave this PR to only cover batching that node.scrollTop forced reflow so multiple element animations aren't expensive, and batching the writes on the next animation frame (or not, depending to browsers) via fastdom. I don't think any of us have the time to rewrite react-transition-group yet, do we? 😅

diagramatics · 2019-05-06T06:17:58Z

Following up on this @STRML to see if your timeline looks any different to the one I have in #385 (comment) for the appear story, or if you have any other solutions to batch the style invalidation 😄

STRML · 2019-05-06T20:32:14Z

We avoid the second reflow in this patch (reflow is only strictly necessary onEntering in my testing), so the timeline looks like this on appear (50 items):

Notice this takes about 8ms, because the addClass is interspersed with forceReflow, causing exactly the kind of DOM thrashing we don't like; however because it's not on the same node but different nodes, it doesn't look like our typical bright-red forced synchronous layout.

While thinking about this, I had the idea to actually push all the nodes needing layout into an array so we ensure that we finish all reads (reflows) before writes, rather than thrash back and forth. The results are encouraging and have no loss in functionality:

This reduces the actual JS-active time from 8ms to about 3ms. If you count the subsequent "Recalculate Style" (which is done synchronously in the first screenshot, rather than in a batch), the total time is about 5ms, a roughly 37% reduction.

…changes Prevents unnecessary context switching for a nearly 40% perf improvement when using "appear" on 50 nodes

STRML · 2019-05-06T20:41:04Z

This is ready for re-test and eventual merge.

diagramatics · 2019-05-09T03:41:40Z

I'm still not convinced we can't use fastdom.measure() like it's supposed to be used, but this works for me. Checked the performance timeline and the thrashing issue is now solved.

@silvenon @jquense I'll leave the reviewing to the maintainers.

diagramatics · 2019-05-27T03:35:04Z

Apologies @silvenon @jquense, pinging again just to go ahead with this :)

jquense · 2019-05-29T17:38:54Z

@taion you have thoughts here?

taion · 2019-05-29T19:07:17Z

I don't really have a view here. I'm quite unfamiliar with fastdom and with what makes DOM manipulations efficient.

jquense reviewed Jul 30, 2018

View reviewed changes

silvenon approved these changes Oct 8, 2018

View reviewed changes

silvenon requested changes Oct 8, 2018

View reviewed changes

jquense closed this Jan 14, 2019

jquense reopened this Jan 14, 2019

diagramatics mentioned this pull request Apr 26, 2019

feat: Use fastdom to batch writes and avoid sync layout (take two) #496

Closed

STRML added 2 commits April 26, 2019 11:57

feat: Use fastdom to batch writes and avoid sync layout

5d0a9ed

refactor: remove visibility

2541850

STRML added 2 commits April 29, 2019 10:49

fix(fastdom): fix animations by triggering reflow

41c08e2

fix(CSSTransition): don't add undefined class

e71fed5

WIthout this patch, try the storybook; you'll see a class of 'undefined' being added to elements on mount because the 'appearClassName' and 'enterClassName' are undefined.

diagramatics reviewed Apr 30, 2019

View reviewed changes

fix(cssTransition): Don't rely on 'active' in className

ec881ae

perf(classHelpers): reflow all pending nodes completely before class …

c0ae113

…changes Prevents unnecessary context switching for a nearly 40% perf improvement when using "appear" on 50 nodes

cdoublev mentioned this pull request May 21, 2019

Add useCssTransitionGroup hook #479

Open

taion mentioned this pull request Mar 19, 2020

TransitionGroup takes 486 seconds to remove 500 components #599

Open

feat: Use fastdom to batch writes and avoid sync layout #385

Are you sure you want to change the base?

feat: Use fastdom to batch writes and avoid sync layout #385

Conversation

STRML commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquense Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olehreznichenko commented Oct 7, 2018

STRML commented Oct 7, 2018

jquense commented Oct 8, 2018

silvenon left a comment

Choose a reason for hiding this comment

silvenon left a comment • edited Loading

Choose a reason for hiding this comment

silvenon commented Oct 8, 2018 • edited Loading

STRML commented Oct 9, 2018 • edited Loading

taion commented Oct 9, 2018

STRML commented Oct 9, 2018

taion commented Oct 9, 2018

STRML commented Oct 9, 2018 via email

STRML commented Jan 14, 2019 via email

jquense commented Jan 14, 2019

jquense commented Jan 14, 2019

diagramatics commented Apr 26, 2019

STRML commented Apr 26, 2019

STRML commented Apr 26, 2019

diagramatics commented Apr 29, 2019

STRML commented Apr 29, 2019

diagramatics commented Apr 30, 2019

diagramatics Apr 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

STRML commented Apr 30, 2019

diagramatics commented Apr 30, 2019

STRML commented Apr 30, 2019

diagramatics commented Apr 30, 2019

jquense commented Apr 30, 2019

diagramatics commented May 1, 2019

diagramatics commented May 6, 2019

STRML commented May 6, 2019

STRML commented May 6, 2019

diagramatics commented May 9, 2019

diagramatics commented May 27, 2019

jquense commented May 29, 2019

taion commented May 29, 2019

jquense Jul 30, 2018 •

edited

Loading

silvenon left a comment •

edited

Loading

silvenon commented Oct 8, 2018 •

edited

Loading

STRML commented Oct 9, 2018 •

edited

Loading

diagramatics Apr 30, 2019 •

edited

Loading