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 with higher numberOfMonths #1473

Open
rachanee-tw opened this issue Jun 22, 2022 · 7 comments
Open

Too slow with higher numberOfMonths #1473

rachanee-tw opened this issue Jun 22, 2022 · 7 comments
Assignees
Labels
In Progress Work in Progress Type: performance Issue or PR related to DayPicker performance
Milestone

Comments

@rachanee-tw
Copy link

rachanee-tw commented Jun 22, 2022

Relevant Issue

#374

Description

When setting the numberOfMonths to a higher value like 10, the performance when selecting dates is really slow, it takes a lot slower than using a lower value like 2.

To reproduce

Fork this CodeSandbox: https://codesandbox.io/s/sandpack-project-forked-pzx9no?file=/App.tsx.

Steps

  1. Try selecting dates and observe how slow the response time.
  2. May change the numberOfMonths to be 1 and compare how much different.
@rachanee-tw rachanee-tw added the Bug Bug or Bug fixes label Jun 22, 2022
@jech33
Copy link

jech33 commented Jul 11, 2022

I tested it and is also slow on mobile devices, this bug will be fixed @gpbl ?

@gpbl gpbl added Type: performance Issue or PR related to DayPicker performance and removed Bug Bug or Bug fixes labels Aug 11, 2022
@gpbl gpbl added the To Investigate Needs more investigation from the mantainers label Aug 19, 2022
@gpbl
Copy link
Owner

gpbl commented Oct 9, 2022

It seems the calls of useDayRender

const dayRender = useDayRender(props.date, props.displayMonth, buttonRef);

are behind the performance issue. From there, we could investigate why these lines are so slow:

const focusContext = useFocusContext();
const activeModifiers = useActiveModifiers(day, displayMonth);
const eventHandlers = useDayEventHandlers(day, activeModifiers);
const selectedDays = useSelectedDays();
const isButton = Boolean(
dayPicker.onDayClick || dayPicker.mode !== 'default'
);
// Focus the button if the day is focused according to the focus context
useEffect(() => {
if (activeModifiers.outside) return;
if (!focusContext.focusedDay) return;
if (!isButton) return;
if (isSameDay(focusContext.focusedDay, day)) {
buttonRef.current?.focus();
}
}, [
focusContext.focusedDay,
day,
buttonRef,
isButton,
activeModifiers.outside
]);

I also suspect we could reduce the calls of isMatch

So far I haven't made any actual progress tho... @jech33 @rachanee-tw could you see why it is that slow?

@oudoulj
Copy link

oudoulj commented Nov 18, 2022

Any update on that performance issue ?
This is something that keeps us from migrating to v8 😿
Thanks again for your work !

Repository owner deleted a comment from github-actions bot Jan 31, 2023
@gpbl gpbl added the In Progress Work in Progress label Feb 4, 2023
@rachanee-tw
Copy link
Author

Still face the issue, any suggestion?

@gpbl
Copy link
Owner

gpbl commented Feb 11, 2023

I've investigated where it goes slow and it is because functions and hooks that are called for each day cell. When rendering a whole year, there are lot of cells to render, thus the performance issue is more visible.

There are for sure workarounds for the existing components, but I'd rather spend the time on something that fixes the issue at the root.

At the moment, I'm playing with the idea where the date calculations would happen at root level (once, before rendering) and not at day level. This would give space for some better debugging (where it is actually slow?) and improvements.

Some other ideas?

@rachanee-tw
Copy link
Author

I investigated and found the similar clue that hooks cause rerendering in multiple levels. I think splitting states to multiple hooks and useMemo may be able to help. But by my low level of understanding of the code base I might be wrong.

@gpbl
Copy link
Owner

gpbl commented Feb 14, 2023

I investigated and found the similar clue that hooks cause rerendering in multiple levels. I think splitting states to multiple hooks and useMemo may be able to help. But by my low level of understanding of the code base I might be wrong.

I've been playing with useMemo, no luck: I suspect some objects are being created at render time (e.g. with newObj = { ...oldObj }, which may cause the slow down and my problem to memoize the calculations. (...calculation that happens for each day, so with memoization we could run into some memory issues)

I think that moving up these function calls (from the Day component to the root), we could have more space for optimization. Then, passing down the state as props would avoid the calls for each day. This mean we can deprecate the hooks.

import { DayPickerProps } from 'DayPicker';

/** Represent the current state of DayPicker */
export interface DayPickerState {
  initialProps: DayPickerProps;
  modifiers: {
    selected: Date[];
    interactive: Date[];
    focused: Date[];
    disabled: Date[];
    hidden: Date[];
    outside: Date[];
    today: [Date];
    rangeStart: [Date];
    rangeEnd: Date[];
  };
  customModifiers: {
    [name: string]: Date[];
  };
  navigation: {
    currentMonth: Date;
    displayMonths: Date[];
    nextMonth?: Date;
    previousMonth?: Date;
  };
}

Thanks @rachanee-tw for the help!

@gpbl gpbl added this to the v9 milestone Apr 14, 2023
@gpbl gpbl self-assigned this Aug 16, 2023
@gpbl gpbl removed the To Investigate Needs more investigation from the mantainers label Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Progress Work in Progress Type: performance Issue or PR related to DayPicker performance
Projects
None yet
Development

No branches or pull requests

4 participants