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

Too slow for mobile #272

Closed
wub opened this issue Jan 30, 2017 · 20 comments
Closed

Too slow for mobile #272

wub opened this issue Jan 30, 2017 · 20 comments

Comments

@wub
Copy link
Contributor

wub commented Jan 30, 2017

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!

@sontek
Copy link

sontek commented Jan 30, 2017

Are you testing with 5.2.0? There were some performance improvements in 5.1.0 that might help you. Its still super slow but it is reasonable to use after the dialog is opened. It takes a couple seconds for the datepicker to open still

@wub
Copy link
Contributor Author

wub commented Jan 30, 2017

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.

@majapw
Copy link
Collaborator

majapw commented Jan 31, 2017

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:

As testament to this, even Airbnb themselves use a completely different component for mobile devices.

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.

@lencioni
Copy link
Contributor

lencioni commented Feb 3, 2017

Depending on where you are seeing the slowness, #286 might help with this some.

@isaachinman
Copy link
Contributor

isaachinman commented Feb 7, 2017

@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 isDayBlocked and isOutsideRange?

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 react-dates results in a situation where the component becomes usable, even for mobile devices.

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.

@wub
Copy link
Contributor Author

wub commented Feb 7, 2017

@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.

@majapw
Copy link
Collaborator

majapw commented Feb 7, 2017

Hi @isaachinman! Thanks for the recommendation. It sounds like it might be a good idea to roll something like this into react-dates, especially if the difference is so dramatic. @moonboots has been doing a lot of perf work for mobile devices so maybe he can look into it.

@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. :)

@isaachinman
Copy link
Contributor

@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 react-dates. In the application I'm currently building, the availability (eg isDayBlocked) can change based on outside parameters, so I clear the moize cache manually in those cases. This kind of functionality might get a little tricky if the cache was housed internally to react-dates.

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 moize.

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, react-dates can end up being your single biggest performance hit, in even large-scale applications. I think whether or not you roll caching into the component depends on who you think your end user is.

@sontek
Copy link

sontek commented Mar 30, 2017

Has any work been done towards this? @isaachinman could you give more details on what you did so I could attempt something similar?

@majapw
Copy link
Collaborator

majapw commented Mar 30, 2017

@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!

@isaachinman
Copy link
Contributor

isaachinman commented Apr 1, 2017

@sontek

First of all, if you are planning on using react-dates in any reasonably-sized codebase, I would highly recommend wrapping it in your own HOC so that you only have to write these things once.

Then it's just a matter of:

export default class DatePickerWrapped extends Component {

  determineBlockedStatus = memoize(day => {

    // Do proprietary calculation stuff here
    const dayIsBlocked = () => false || true

    return dayIsBlocked

  })

  render() {

    return (
      <DateRangePicker
        {...this.props}
        isDayBlocked={determineBlockedStatus}
      />
    )
  }
}

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.

@majapw
Copy link
Collaborator

majapw commented May 2, 2017

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 CalendarDay objects instead of all of them.

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. :)

@majapw majapw closed this as completed May 2, 2017
@pavle-lekic-htec
Copy link

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.

@pavle-lekic-htec
Copy link

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.

@ljharb
Copy link
Member

ljharb commented Jun 14, 2018

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.

@pavle-lekic-htec
Copy link

@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.

@pavle-lekic-htec
Copy link

Btw, with React dev tools I see that some things are being diffed needlessly.

@ljharb
Copy link
Member

ljharb commented Jun 14, 2018

If that’s the case, we’d really appreciate a PR to improve that.

@pavle-lekic-htec
Copy link

I cannot promise a pull request because I don't have much free time, but I can share a screenshot with dev tools turned on so that you can see the issue. When I move a mouse over a day everything is being diffed, even the months that are not visible on the right.
diffing

@isaachinman
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants