Draft: Improve performance for selecting long date ranges

[rodgobbi]

What's Changed

Update the select function in the src/selection/useRange.tsx hook to improve the performance.

Type of Change

• [X] Bug fix
• [ ] New feature
• [ ] Breaking change
• [ ] Documentation update

Additional Notes

Fixes #2531

[rodgobbi]

Hi @gpbl

I created a draft PR for now because I'd like to discuss with you what I found and align with you on how to implement the rest.

There's a straightforward improvement we can do with these changes. I hoisted the check for excludeDisabled && disabled to the upper if, which prevents the while loop from running for all use cases where excludeDisabled is not enabled or there's no disabled dates.

But we still have the performance issue when excludeDisabled && disabled is true. I checked the type of the disabled prop, which is the Matcher type, and it has many possible formats, which makes it a bit difficult to fix the performance issue. 😅 I expect though that it is possible to fix the issue for all possible Matcher formats besides the callback one which is (date: Date) => boolean. I think we can handle most of the Matcher formats by having a unique check for each format that is the most performant for that specific format. E.g.: if disabled contains a single Date, we could just check if that Date is within the selected DateRange using the ideal function from date-fns for this use case. But if the Matcher is a (date: Date) => boolean function, I think we cannot do anything besides iterating every date within the selected DateRange, as we need to call the Matcher with all the iterated dates.

Do you think it's worth adding this complexity to solve this issue?

If yes, I can work on it and show it to you once it's ready. I could also do it in a separate PR, as these changes I mentioned could be merged separately.

[gpbl]

Nice work, @rodgobbi! I believe the change is indeed a step forward. However, this line is where most of the impact occurs. Specifically, it's looping from newRange.from to the existing newRange.to, which could be quite a big range.

I suspect that using months[months.length-1].month (months is coming from useCalendar) instead of newRange.to might be enough to reduce the loop's size.

How could we test this behavior in Jest? The number of calls of... what? It would simplify debugging.

[gpbl]

PS A suggestion from ChatGPT to improve the loop:

You can improve the performance by moving the expensive differenceInCalendarDays calculation outside of the loop, checking once before entering the loop. Additionally, reducing the complexity of dateMatchModifiers if possible can further improve performance. Here’s an optimized version:

if (newRange?.from && newRange.to) {
  let newDate = newRange.from;
  const totalDays = dateLib.differenceInCalendarDays(newRange.to, newDate);

  for (let i = 0; i < totalDays; i++) {
    newDate = dateLib.addDays(newDate, 1);
    
    if (excludeDisabled && disabled && dateMatchModifiers(newDate, disabled, dateLib)) {
      newRange.from = triggerDate;
      newRange.to = undefined;
      break;
    }
  }
}

[rodgobbi]

@gpbl Thanks for the suggestion, I missed hoisting the differenceInCalendarDays call, which is indeed better.

Just one important detail, we cannot rely only on the visible months returned by the useCalendar hook for checking if there is any disabled date within the selected DateRange, because the selected DateRange can be longer than the visible months in the calendar.

I'll work on what I have in mind during the upcoming week and if it works, we can review it when it's ready.

About

How could we test this behavior in Jest? The number of calls of... what? It would simplify debugging.

IMO testing how many times something was called doesn't provide that much value, as different devices take different amounts of time to run the same amount of iterations. It's also difficult to assert performance, even more difficult using only Jest. For such a test I imagine we could assert how long it took for the component to rerender. The problem is that every different machine running this test will have a different result, which can cause the test to be very flaky. It seems like it's a case where the cost of testing is not worth it, but let me know if you'd like to try implementing a test for it.

[gpbl]

Just one important detail, we cannot rely only on the visible months returned by the useCalendar hook for checking if there is any disabled date within the selected DateRange, because the selected DateRange can be longer than the visible months in the calendar.

This is true! The reason we do that is to reset the range to start from the clicked date.

🤔 We have another way to check if a day is disabled via useDayPicker().getModifiers(). This function returns the modifiers for a given day and is more performant than dateMatchModifiers.

The problem is that every different machine running this test will have a different result, which can cause the test to be very flaky.

This could be my chance to finally add the long-awaited performance tests to DayPicker 👯

[rodgobbi]

@gpbl Great work! I see that you already set up the performance tests, that was quick 👏

Two points about it: 1 - Currently the performance CI check is failing in this PR, and it doesn't look like it's related to the lighthouse test.

2 - In theory the RangeLong.tsx example should not be passing the performance test, was it passing when you added the performance CI check for it? If yes, we will need to use a much longer date range in the test case or try to limit the performance of the process running the test. The second option looks better if feasible, as it would make the CI check less flaky.

[gpbl]

@rodgobbi Cool! I was excited to add the performance tests, and I played around with Lighthouse over the weekend.

I’m learning how to set up Lighthouse with GitHub correctly—sorry for the confusion with the PR. I’ve just updated it to the main branch.

I still need to write the documentation for this new performance-tests package. It should also work via the CLI and speed up the testing a little bit:

pnpm install
pnpm --filter performance-tests run capture

For now, I’d use these performance tests as a report here on GitHub. The job runs when a PR is labeled with performance, and the report can be downloaded as an artifact. Ideally, I’d like to see the report as a comment in the PR.

[rodgobbi]

@gpbl I finished the first iteration of what I had in mind, I added the src/utils/rangeMatchModifiers.ts utils function.

I tested the "Interaction to next paint" with the examples/RangeLongExcludeDisabled.tsx component with 20x CPU slowdown and this is the difference:

Current implementation	rangeMatchModifiers implementation

Important note, ~ 300 ms of response time is the default expected time for the component in general when using 20x slowdown (on my machine):

@gpbl Let me know what you think and if you want to integrate those changes, I pushed the first iteration I worked on to showcase it, so I still need to test better, polish the code, and implement automated tests.

[gpbl]

OK, I had a chance to dig deeper into your PR, @rodgobbi!

The performance improvement is noticeable, and it's definitely a step in the right direction. However, the part about matching ranges instead of single days is still a bit unclear to me, and I'm not sure I fully understand what the bottleneck was there.

I'll try adopting some of your changes in a different branch to see how it goes.

[rodgobbi]

@gpbl thanks for following up! Great, lemme know what you think after using this solution more. I also found another issue while testing the react-day-picker, I'll open another issue later to discuss, which can be tackled in parallel.

However, the part about matching ranges instead of single days is still a bit unclear to me, and I'm not sure I fully understand what the bottleneck was there.

Could you elaborate more about your concerns and doubts so I can explain better the solution?

If I understood correctly your concern, the difference and also the benefit of creating a custom function for matching Matchers against a date Range is that for most types of Matchers we can have a specialized logic that requires way less computation than iterating all days in the date Range and calling dateMatchModifiers with every iterated day date.

E.g.: if the day picker has the date 31/12/1999 disabled, and we try to select the date range from 01/01/0001 to 01/01/2000, the previous implementation will call dateMatchModifiers approximately 729.635 times (1999 years * 365 days) until the logic finds that one of the days in the date Range is disabled so the selected date Range is invalid. With the rangeMatchModifiers implementation, we call rangeIncludesDate only once for the same use case, and rangeIncludesDate internally calls differenceInCalendarDays and isSameDay a fixed amount of times no matter the duration of the date Range thus the computational complexity is fixed no matter how long the date Range passed to rangeMatchModifiers is.

The example above helps illustrate that the computation grows with the duration of the date Range if we only use single days for matching against Matchers, which can eventually hit a bottleneck taking too long to iterate all days in a date Range. However, I could not find a better solution when the Matcher is a function, because the Matcher needs to be tested against all single days within the date Range.

[rodgobbi]

@gpbl No problem! It's good to be thorough and I appreciate the care you put into maintaining the lib.

Thanks for offering the help, I'll try to handle everything now that we are aligned, I'll work on it this week and let you know once it's ready for review. FYI, I'll leave some replies to your comments.

Also, one last thing that I need feedback: I left this code comment for discussion. Because the performance bottleneck can still happen for Matcher functions, do you think it would be useful to leave a console warning when we detect that this while loop took too long to complete? This would help in debugging if someone ever faces this issue, we could instruct them to try to avoid Matcher functions and use other Matcher types if possible. Detecting it would be easy, we could measure if, for example, the while loop took more than 100ms to complete.

[rodgobbi]

@gpbl I got in the flow today after work and finished the changes, the PR is ready for review 👌 I implemented many tests for everything I added, and thanks to them I was able to surface some bugs in the implementation of the rangeContainsModifiers function.

[gpbl]

Looking great, @rodgobbi! Thanks for your effort and patience in addressing my feedback.

Your contribution is the first significant one in a while. I’m happy to see this performance bottleneck so elegantly resolved. 🤟

[rodgobbi]

@gpbl Thanks for the feedback and encouragement! I'm happy to be able to contribute back 🙏

As a follow-up, I found a minor bug with the day picker when using the startMonth and endMonth props. Do you prefer that I open an issue first, reporting the bug, or is it better to open a PR directly? (I'd explain the issue in the PR description)

[gpbl]

@gpbl Thanks for the feedback and encouragement! I'm happy to be able to contribute back 🙏

As a follow-up, I found a minor bug with the day picker when using the startMonth and endMonth props. Do you prefer that I open an issue first, reporting the bug, or is it better to open a PR directly? (I'd explain the issue in the PR description)

Sure, open the PR - do not worry about creating an issue if it is a quick fix. Thanks!