-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Too slow for mobile #272
Comments
Are you testing with |
Thanks, @sontek. Yep, I'm using the latest version. We've moved to another one that suits our needs; it's much faster, and much lighter. |
Hi @wub, thanks for the feedback. We're continuously improving the perf and working to get this into a state we're happy with on mobile. Would love to take any contributions you have in the future. Can you provide any information on the range of devices your testing team looked at? As for this statement:
This is the legacy datepicker that we are working to replace. This is a testament to nothing beyond the fact that we haven't had the bandwidth to implement and land on a mobile design we are happy with. This is likely to change soon. |
Depending on where you are seeing the slowness, #286 might help with this some. |
@wub Can you explain (or show examples) of how you memoized functions? I assume you're referring to the methods that are getting called thousands of times, like While mobile is less of a concern for me, I believe memoization and user-provided performance enhancement is unfortunately necessary to make this component usable. Update: Happily, I've discovered that using moize in conjunction with I would highly recommend this become "suggested" use, or at least added to the docs as an example. The difference is astounding, absolutely night and day. @wub I think something similar should solve your problem. I tested on a few different mobile devices and had no problems. |
@majapw, @lencioni, @isaachinman: thanks for the feedback. @majapw - We experienced these slowdowns on the iPhone 5S, and the iPad 2. I understand these are old devices, but sadly they represent a sizeable portion of our userbase. @isaachinman - Thanks for the heads up on moize, I'll look into it and report back. I can see it being useful for all kinds of functional stateless components, actually. |
Hi @isaachinman! Thanks for the recommendation. It sounds like it might be a good idea to roll something like this into @wub No problem! We also have a lot of users on older devices who we have... not been very good to in the past and whose experience we are definitely trying to improve. We are working on getting the performance of this library in a much better state so hopefully it'll be able to serve your usecases in the near future. :) |
@wub Let me know what you find, I think you'll see the difference is dramatic. (And yeah, if your components are truly functional and render more than a few times, they will always benefit from caching.) @majapw Yeah, I was going to suggest that, but actually realised that even in my own use case, I would prefer to manage any memoization/cache outside of That being said, I'm happy to work on a PR that implements memoization/caching, or just PR an example of how it can be done with There are a lot of things I like about this project, but performance has been more or less left up to the end user, and if you don't do things a certain way, |
Has any work been done towards this? @isaachinman could you give more details on what you did so I could attempt something similar? |
@moonboots has been exploring a lot of perf work ahead of us landing this on mobile. I have a few ideas as well. There's a bit of a 🔥 under our asses now, so hopefully there will be a number of improvements coming soon! |
First of all, if you are planning on using Then it's just a matter of:
Assuming your function is pure, this will cache its results in memory instead of performing (potentially) taxing calculations over and over, needlessly. In my case, the calculations being performed weren't anything particularly crazy, but the datepickers were laggy on desktop and unusable on most mobile devices. After implementing a cache, it's totally fine on both. As I've said before, happy to write something up for the docs, or indeed submit a PR to include caching by default. |
v11.0.0 (I swear the last breaking release for a while) rearchitects modifiers so now there's about the same overhead at mount, but then calendar interactions only updates the relevant You can see more details in #450, but in the meantime, I'm going to close this issue of being "generally slow". There are still some improvements to be made, notably during the month transition phase, but I think this new architecture opens up a lot of doors. :) |
The main reason why your calendar is slow is because you're not using React.PureComponent, so everything is being diffed on every change and also because of heavy use of moment.js which is terribly slow. You can do a lot of calculations using timestamps, which are integers and super fast for manipulation, for example if you need next day you could just add 24 * 60 * 60 to current timestamp, or if you need previous week just subtract 7 * 24 * 60 * 60 etc. Hope that helps. |
You could use standard (built in) Date object, that's also a lot faster than Moment.js. But again, I'm pretty sure you'll get best performance if you switch to integer arithmetics via timestamps. |
Those sorts of calculations are often wrong - dates are much much more complex than that. Faster performance at the expense of correctness isn’t better for anyone. As for PureComponent; we’re using shouldComponentUpdate, which is all that PureComponent is sugar for, so we’re already using it for all intents and purposes. |
@ljharb I am aware of complexities with date, I wrote a calendar before, also used moment.js so I know the issues. But for example you could use Moment.js to calculate first day, and then to get other days do calculations on integers by adding 24 * 60 * 60 to the first "correct" date. Or something similar, it doesn't need to be this way obviously it depends from case to case. |
Btw, with React dev tools I see that some things are being diffed needlessly. |
If that’s the case, we’d really appreciate a PR to improve that. |
This is a very old issue and I doubt that we're discussing the same thing. @pavle-lekic-htec it might be worth opening a separate issue. |
The testing team at work have just (sadly!) rejected my branch because of how slow this is on mobile. Even after memoization, hacks, etc, I can't make this fast enough for anything but the latest devices, even if I drop in the vanilla picker.
I did notice that Airbnb themselves use a different component for mobile devices.
Anything I can do about this?
Should there be a note somewhere in the README, making sure people test on older devices? It's important to know what a potentially large percentage of your userbase will experience.
Once the project is done, I'll loop back and see if I can contribute some performance enhancements.
Cheers for all the hard work guys!
The text was updated successfully, but these errors were encountered: