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

• 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]

Well done, @rodgobbi! I can see the performance improvements. Give me some time to review your changes. Thanks!

[gpbl]

Interesting... if this is the cause of the slowness, why not refactor this into a utility that works regardless of whether it's dealing with ranges or not?

[rodgobbi]

If I understood the suggestion correctly, I think this is the case with the current implementation, and the dateMatchModifiers utils function is already the generic function that validates any specific Date against a list of Matchers, regardless of the Date coming from a date Range.
This is easier to understand, has less complexity, and works, but hits a performance bottleneck that the rangeMatchModifiers solves.
Having a logic that specifically handles the Range data structure allows us to optimize for that.

The only issue is that I don't see any other possible approach when a Matcher is of type (date: Date) => boolean, we need to iterate all dates within the date Range passing the dates to the Matcher function.