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

fix: improved performance speed for getOrderOfEvent and getCountOfEventsAtEvent methods #1124

Merged

Conversation

arisyo13
Copy link
Contributor

@arisyo13 arisyo13 commented Dec 5, 2024

I used rndemo project in order to understand why modes of week day and 3days where very slow when trying to navigate them.
To be able to render 87 events on week mode it was taking approx 1260ms
I applied to stress it even further with 500 events which took a bit over a minute 62seconds.

Of course this makes the library almost unusable since in all real life scenarios we will navigate from a screen to this one and that could take some time to render.

After i applied the proposed fixes

  • 87 events loaded in just 88 milliseconds
  • 500 events loaded in just 430 milliseconds

Copy link

vercel bot commented Dec 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-big-calendar ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 5, 2024 3:19pm

Copy link

vercel bot commented Dec 5, 2024

@arisyo13 is attempting to deploy a commit to the Kazuya Gosho's projects Team on Vercel.

A member of the Team first needs to authorize it.

Comment on lines +160 to +166
expect(index).toEqual(2)
})

test('3 events end', () => {
const event = events[5]
const index = utils.getOrderOfEvent(event, events)
expect(index).toEqual(2)
expect(index).toEqual(1)
Copy link
Contributor Author

@arisyo13 arisyo13 Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw as you can see i have adjusted the tests here to satisfy new changes.

Although as you will correctly assume that the changes that i did possibly broke the functionality.

To be able to understand why, I took the mocked events from tests and used them in rndemo project.
This way i am able to visualise them in the screenshots below. (i adjusted year from 2021 to 2024 so i don't have to scroll forever to get there)

Screenshot is before changes
image

Screenshot after changes
Screenshot_1733403264

As we can see we have three overlapping events which need to be sorted based on chronological order.

Let's analyse the events from testEvents:
"Repair my car" (2024-09-18T05:45 to 2024-09-18T11:30)
"Meet Realtor" (2024-09-18T06:25 to 2024-09-18T07:55)
"Laundry" (2024-09-18T06:25 to 2024-09-18T09:00)
These three events overlap with each other.

Sorting them in order of start time:

"Repair my car" (Start: 05:45, End: 11:30)
"Meet Realtor" (Start: 06:25, End: 07:55)
"Laundry" (Start: 06:25, End: 09:00)
Thus, the sorted order of overlapping events is:

Repair my car (2024-09-18T05:45 to 2024-09-18T11:30)
Meet Realtor (2024-09-18T06:25 to 2024-09-18T07:55)
Laundry (2024-09-18T06:25 to 2024-09-18T09:00)

Default Sorting Criteria for Overlapping Events:
Primary Criterion: Start Time
Events are primarily sorted by their start time (earlier start time comes first).

Secondary Criterion: End Time (Tie-Breaker)
When two events have the same start time, the event with the earlier end time takes precedence. This ensures shorter events come first.

Applying this to "Meet Realtor" and "Laundry":
Start Times: Both start at 06:25, so we move to the secondary criterion.
End Times:
"Meet Realtor" ends at 07:55.
"Laundry" ends at 09:00.
Since "Meet Realtor" ends earlier, it is sorted first.

@acro5piano
Copy link
Owner

@arisyo13 Thank you very much for the PR! The binary search and early break is genious. Checked the storybook and working fine. Let me merge this!

@acro5piano acro5piano merged commit fb1b7ca into acro5piano:main Dec 7, 2024
3 checks passed
@acro5piano
Copy link
Owner

Released on v4.16.1

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

Successfully merging this pull request may close these issues.

2 participants