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.

Sign up for GitHub

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

Jump to bottom

replace Cache by Lru cache #2451

Merged
merged 1 commit into from
Jan 31, 2025
Merged

replace Cache by Lru cache #2451

merged 1 commit into from
Jan 31, 2025

Conversation

ftoromanoff
Copy link
Contributor

@ftoromanoff ftoromanoff commented Oct 24, 2024 •
edited
Loading

replace our own itowns.cache per the lru-cache package.

Currently we use 1 cache for each Layer + 1 for Style and another one for tile. Should we change that ?
I also arbitrary fixed the max size of each cache to 500. Maybe this should be discuse, depending of the type of cache ?

Other question what should I do with export const CACHE_POLICIES, I currently kept the 'Cache.js' file just for this.

@ftoromanoff ftoromanoff force-pushed the lruCache branch 3 times, most recently from 217ac00 to f5e0a65 Compare October 24, 2024 13:27
@ftoromanoff ftoromanoff reopened this Oct 24, 2024
@ftoromanoff ftoromanoff force-pushed the lruCache branch 3 times, most recently from 3611195 to 755b70e Compare October 24, 2024 15:02
@ftoromanoff ftoromanoff force-pushed the lruCache branch 2 times, most recently from 70d6cf4 to a615509 Compare October 25, 2024 13:19
@jailln
Copy link
Contributor

jailln commented Nov 15, 2024

@ftoromanoff does it solve #869 ?

@ftoromanoff ftoromanoff force-pushed the lruCache branch 9 times, most recently from 119976e to 51d9192 Compare January 24, 2025 09:05
@ftoromanoff
Copy link
Contributor Author

@ftoromanoff does it solve #869 ?

I can't say. Is there a way to test it ?

@ftoromanoff ftoromanoff marked this pull request as ready for review January 24, 2025 09:45
@AnthonyGlt AnthonyGlt self-requested a review January 24, 2025 10:30
@jailln jailln requested review from AnthonyGlt and removed request for jailln and AnthonyGlt January 24, 2025 14:27
AnthonyGlt
AnthonyGlt reviewed Jan 27, 2025
View reviewed changes
Copy link
Contributor

@AnthonyGlt AnthonyGlt left a comment

Choose a reason for hiding this comment

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

Thank you for this integration, it looks nice 👍

The refactoring looks good to me, well done

@AnthonyGlt
Copy link
Contributor

Is there some improvements from these changes ? Some data that could explain this choice ?
Thank you

@ftoromanoff
Copy link
Contributor Author

Is there some improvements from these changes ? Some data that could explain this choice ? Thank you

  1. better performance when using an LRU cache.
  2. No more maintenance of this module
  3. We now have a memory bound for the cache

@ftoromanoff ftoromanoff force-pushed the lruCache branch 2 times, most recently from 049c40e to 16ecb3d Compare January 28, 2025 14:19
@ftoromanoff ftoromanoff mentioned this pull request Jan 31, 2025
Draft
@ftoromanoff @Desplandis
refactor(cache): use LRUCache instead of own cache
cf0fba2 
BREAKING CHANGE:
- remove Source#onParsedFile callback
@Desplandis Desplandis merged commit 6d12fde into iTowns:master Jan 31, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AnthonyGlt AnthonyGlt AnthonyGlt approved these changes

@Desplandis Desplandis Awaiting requested review from Desplandis

@HoloTheDrunk HoloTheDrunk Awaiting requested review from HoloTheDrunk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ftoromanoff @jailln @AnthonyGlt @Desplandis