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

Make remove key O(log N) instead of O(N^2) #360

Merged
merged 2 commits into from
Sep 22, 2023

Conversation

Szymon20000
Copy link
Contributor

@Szymon20000 Szymon20000 commented Sep 22, 2023

I profiled app start with account provided by @danieldoglas. Turned out that remove key from cache takes O(n^2) where n is number of keys we have in cache which is set to 10k. The reason for that is that _.difference(storageKeys, recentlyAccessed); is quadratic. It calls filter method and pass there lambda that calls contains.
https://github.com/jashkenas/underscore/blob/d12221366eacbb63e37cff328bed475f34ebe083/modules/difference.js#L8
Screenshot 2023-09-22 at 10 55 53

Instead we now make use of BTS structure which set is and nice property that we can access first element in O(log n) time.
Screenshot 2023-09-22 at 10 58 27
Before ^
Screenshot 2023-09-22 at 10 58 49
After ^

Details

Related Issues

GH_LINK

Automated Tests

Linked PRs

@Szymon20000 Szymon20000 marked this pull request as ready for review September 22, 2023 09:00
@Szymon20000 Szymon20000 requested a review from a team as a code owner September 22, 2023 09:00
@melvin-bot melvin-bot bot requested review from Beamanator and removed request for a team September 22, 2023 09:00
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

This makes sense to me, thanks!

@marcaaron marcaaron removed their request for review September 22, 2023 09:17
@tgolen tgolen merged commit e29e678 into Expensify:main Sep 22, 2023
3 checks passed
@ospfranco ospfranco mentioned this pull request Sep 22, 2023
59 tasks
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.

4 participants