Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Two small code changes with (hopefully) a major perf impact. #745

Closed
wants to merge 6 commits into from

Conversation

redders6600
Copy link

I noticed perfectscrollbar doesn't run perfectly on mobile or low performance PCs, so have tried to make a couple of simple optimisations.

DISCLAIMER - our use-case doesn't cover all of the functionality of PS, so I'm not 100% sure there aren't regression bugs, but I couldn't find any, plus I think the changes are minimal.

Basically I am just wrapping the exported function from update-geometry.js in requestAnimationFrame, caching the last value it was called with. This ensures the code runs at the right time (at the start of the frame), and also limits the frequency at which it is called not to be faster than the browser can render it. I have also added a will-change: transform property to the CSS - this promotes the scrolling area to a layer, which allows it to be repainted separately (or something like that, I'm not an expert!).

On a Galaxy S8 with a medium sized scrolling list, these two changes resulted in an increase from about 20fps to about 50fps average. With just one of two changes (either/or), the increase was up to about 30-35fps. I would be interested to see the numbers from other people's devices.

Note the following:

  1. I haven't changed the /dist or version or anything - assume that's something @utatti will do.
  2. I haven't tested extensively on IE/Safari/etc
  3. I didn't provide a fiddle - this is mainly because I wasn't sure how to import the modified library without updating the dist folder in my own repo.

Comments & input very welcome!

  • Read CONTRIBUTING.md
  • Run npm test to make sure it formats and build successfully
  • Provide the scenario this PR will address(some JSFiddles will be perfect)
  • perfect-scrollbar JSFiddle
  • Refer to concerning issues if exist

- Added requestAnimationFrame wrapper to the updateGeometry module
- Added will-change transform to the css wrapper class to promote
  to layer.
@DimitarNestorov
Copy link

Just wrapping updateGeometry in requestAnimationFrame isn't enough. Measurement and mutation must be in separate requestAnimationFrames (https://github.com/wilsonpage/fastdom).
I am suggesting this PR to be rejected and I will recreate it by implementing fastdom inside updateGeometry, which should bump the fps to 60.

@hatashiro
Copy link
Contributor

@DimitarNestorov Measurement cannot always be separated into another frame because PS sometimes decides whether preventDefault or stopPropagate should be called by the result of the measurement. Is there a nice way to resolve the case?

@redders6600
Copy link
Author

redders6600 commented Jan 18, 2018

@DimitarNestorov Happy for that approach - I haven't had loads of time to work on this, but noticed there were a couple of quick wins that I could implement in 10 mins, so I did!

One approach is to reject and re-implement fully (a task I personally don't have the resources to complete myself) another is to merge because this is better than the current state (no requestAnimationFrame at all), and create an issue/task for someone to upgrade at a later date.

Given there was an issue created and subsequently closed on this due to lack of resource, my feeling is that an interim 'better than nothing' approach is definitely more beneficial.

I am now using my fork in my project, so I'm not really bothered either way, just thought others might benefit :)

EDIT just re-read and realised you are proposing to re-create the PR yourself - I initially missed that part. I'm fine with that! 💃

@DimitarNestorov
Copy link

@utatti requestAnimationFrame is somewhat similar to setTimeout and setInterval in the sense there is a clear method (cancelAnimationFrame). My idea is whenever no mutation has to happen (preventDefault/stopPropagation), I am just going to add cancelAnimationFrame for the mutation callback. fastdom also implements a clear method.

Let's keep this PR open so that we can benchmark both implementations to see which one actually performs better.

@DimitarNestorov
Copy link

A turn of events has occurred. It seems I was wrong that measurements and mutations must happen at different requestAnimationFrames. But batching measurements to be before mutations inside a requestAnimationFrame is what gets max performance. I looked at the source of fastdom to be surprised that measurements and mutations weren't in different rAFs.

Splitting updateGeometry into 2 rAFs won't yield a performance bump. What would is wrapping every other function that does DOM measures and mutations in a rAF. However rAF is similar to setTimeout in the sense that the callback is going to be called asynchronously and in event handlers we need the result immediately in order to decide whether to call event.preventDefault. I am going to start thinking of a solution for the event.preventDefault case.

Since all the measurements in updateGeometry are in the first 5 lines and everything after that is a mutation it's at max performance as it is.

Simply put: I would love to see this PR merged.

More on the topic: wilsonpage/fastdom#35

Ed Hinchliffe added 2 commits February 1, 2018 13:09
- Fixes bug where only the last instance to request a geometry
  update would actually have its geometry update.
- Only trigger one geometry instance per update per frame
@redders6600
Copy link
Author

I updated my branch today - I noticed that my last implementation would only cache a single instance's request to update geometry. This caused issues when there are multiple instances of perfect scrollbar on a page whereby if both needed a geometry update (e.g. resize, initial load, etc.), only the last one to register call the requestAnimationFrame function would actually update.

Furthermore, multiple requestAnimationFrame listeners would be added, meaning the function could get called many times for a single instance at the start of a frame. I think the two commits I've just made fix this.

@filipkappa filipkappa closed this Mar 23, 2022
@mdbootstrap mdbootstrap locked and limited conversation to collaborators Mar 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants