gilbox / react-track

Track the position of DOM elements. Create cool animations.
340 stars 23 forks source link

debouncing #1

Closed appsforartists closed 8 years ago

appsforartists commented 9 years ago

It seems like you're thrashing the DOM with recalcs by calculating rects directly in a scroll listener. Have you considered sampling the DOM less often (perhaps with requestAnimationFrame)?

gilbox commented 9 years ago

Yes, I considered and even tested it out one time. However, in my test there was no performance difference. I decided not to prematurely optimize. If you can come up with a test case where it matters I'd be happy to check it out.

hydrotik commented 9 years ago

I'm starting to see this myself as well, especially when mixing with something like react-scroll (which I would expect as there are potentially two scroll based listeners running in parallel).

gilbox commented 9 years ago

@hydrotik do have any detail about performance problems?

hydrotik commented 9 years ago

I have a couple parallax windows using the example code from the demo. I also have some links and element containers from react-scroll and when I click on them I see some jitter. It looks like read-scroll is using the requestAnimationFrame https://github.com/fisshy/react-scroll/blob/master/modules/mixins/animate-scroll.js If you want I can post the code as well from https://github.com/hydrotik/node-hapi-react-redux-sass-typescript-mongo-webpack-hmr-gulp

gilbox commented 9 years ago

Could you try doing a simple experiment to see if it would really make a difference? you could edit node_modules/react-track/index.js directly... maybe something like this...

  componentDidMount() {
   var tick = event => {
      this.setState({ rect: document.documentElement.getBoundingClientRect() });
      requestAnimationFrame(tick);
    };
    requestAnimationFrame(tick);
  }

This will fire continuously but might be useful in any case.

hydrotik commented 9 years ago

Ok so I replaced the code here and did some tests between the two render methods:

value: function componentDidMount() {

      var _this = this;

      //*
      window.addEventListener('scroll', function (event) {
        _this.setState({ rect: document.documentElement.getBoundingClientRect() });
      });
      //*/

      /*
      var tick = function(event) {
        _this.setState({ rect: document.documentElement.getBoundingClientRect() });
        requestAnimationFrame(tick);
      };
      requestAnimationFrame(tick);
      */
    }

There seemed to be a perceived improvement when using requestAnimationFrame(tick); but it wasn't completely obvious. Using Chrome tools I captured painting when clicking the editorial button which animates the scrolling of the page to an editorial block below the slick carousel which is through the tracked areas. scroll_test

I'd like to spend a little more time creating a consistent test between the two, but so far this is what I found. Not sure if anything jumps out?

gilbox commented 9 years ago

What you're looking for is multiple renders in a single frame. If you re-run the test, then zoom in on the graph you will see that the graph clearly indicates the start and end of the frame. Your screenshot is too zoomed out to know for sure, however the 2000ms - 4000ms range looks pretty dense. On the other hand, the top graph is evenly spaced (as we'd expect).

If possible, please try the test again and zoom in on both graphs and look for multiple renders in any single frame.

There are also some surprising spikes in the top graph, indicating a drastic drop in frame-rate. If that happens again, zoom in on the spikes and try to discern what's going on there.

gilbox commented 9 years ago

There seems to be a need, so I implemented debounce in a branch named debounce. I'll probably merge it into master after some more testing.

hydrotik commented 9 years ago

Also noted this in my test which I forgot to mention, but I will check the branch.

82app.0.1.0.js:12448 Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op. Please check the code for the undefined component.
gilbox commented 9 years ago

That makes sense, I was forgetting to remove the scroll event listener. However, it should be fixed in the debounce branch.

hydrotik commented 9 years ago

I'm looking at a couple possible solutions for consistently measuring the performance, but if anyone has any suggestions on something they have worked with? Seems like measuring paint data is the most important indicator of optimizing the performance?

gilbox commented 9 years ago

I'm not sure what you mean by paint data. To determine if something needs to be debounced, confirm that multiple un-batched renders occur in a single frame. You can find this out simply by zooming in on the graph.

gilbox commented 8 years ago

debouncing is now merged into v0.3.1 https://github.com/gilbox/react-track/pull/6