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

perf(core): binary search for findIndex #493

Merged
merged 14 commits into from
Oct 13, 2024
Merged

Conversation

onx2
Copy link
Contributor

@onx2 onx2 commented Aug 10, 2024

This is a quick win that should help with findIndex performance. RE: #488

Here are a few benchmarks that I ran to get an idea of the perf diff (MS averaged over 10,000 iterations)

Cache size: 10k | Item size: 90 | offsetToFind: 5000
image

Cache size: 1,000 | Item size: 45 | offsetToFind: 900
image

Cache size: 100 | Item size: 15 | offsetToFind: 99
image

@onx2 onx2 force-pushed the onx2/testing-perf branch 2 times, most recently from 5249b36 to 2491d1e Compare August 10, 2024 17:34
Update src/core/cache.ts

remove formatting commas an prefer slice

wip
@onx2 onx2 marked this pull request as ready for review August 10, 2024 17:48
@onx2 onx2 changed the title feat: binary search for findIndex perf: binary search for findIndex Aug 12, 2024
@onx2 onx2 changed the title perf: binary search for findIndex perf(core): binary search for findIndex Aug 12, 2024
src/core/cache.ts Show resolved Hide resolved
@onx2
Copy link
Contributor Author

onx2 commented Aug 22, 2024

@inokawa I made some updates I thought were sensible, mind taking a look? If you're okay with these changes, I'll update the tests - or I can revert back to the previous behavior.

  • jsdocs so it is easier for contributors to understand what is happening in these internal fns
  • input arg validation in findIndex
  • -1 return when invalid input or no index found in findIndex
  • handle invalid cases in computeRange

@inokawa
Copy link
Owner

inokawa commented Aug 22, 2024

Could you fix the logic so that the tests pass?

@onx2
Copy link
Contributor Author

onx2 commented Aug 22, 2024

I went ahead and reverted those changes. I think there will either need to be a breaking change or more complex logic to meet the test criteria than is probably worth to change the return value of findIndex

@onx2
Copy link
Contributor Author

onx2 commented Aug 28, 2024

Friendly ping on this so it doesn't get out of date with main. @inokawa Do you have anything you'd like me to change or test before merging in?

@inokawa
Copy link
Owner

inokawa commented Aug 28, 2024

Thank you for pinging! Basically I think it's ok if PR passes all of the current tests because they cover the most of edge cases. However I'd like to check it later when I have time, to publish it without any regression.

@onx2
Copy link
Contributor Author

onx2 commented Sep 13, 2024

Thank you for pinging! Basically I think it's ok if PR passes all of the current tests because they cover the most of edge cases. However I'd like to check it later when I have time, to publish it without any regression.

👋🏼 Hello again! I don't mean to be a bother but would like to see this merged. Is there anything I can do to help test to get the level of confidence required for a merge?

I ran the storybook and tested a quite a few of the stories without issue but IDK if there is something specific you'd like to test. Happy to help wherever I can, I know it is very time consuming maintaining OSS!

Copy link
Owner

@inokawa inokawa left a comment

Choose a reason for hiding this comment

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

Thanks! And sorry for my late reply.

@inokawa inokawa merged commit ce55ae5 into inokawa:main Oct 13, 2024
4 of 5 checks passed
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