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

Make the cache configurable by cpp API #950

Merged
merged 16 commits into from
Feb 27, 2025
Merged

Make the cache configurable by cpp API #950

merged 16 commits into from
Feb 27, 2025

Conversation

mgautierfr
Copy link
Collaborator

This fix #946.

Contrary to what proposed in #946, this doesn't introduce a cache config structure but use simple method:

This PR probably have some small conflict with #949, I will resolve them as soon as a PR is merged.

@mgautierfr
Copy link
Collaborator Author

CI seems to be broken for other reasons than this PR.

veloman-yunkan
veloman-yunkan requested changes Feb 10, 2025
View reviewed changes
veloman-yunkan
veloman-yunkan reviewed Feb 11, 2025
View reviewed changes
Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

Let's have #949 merged first. I will review the rebased&fixed-up version of this PR again after conflict resolution.

@mgautierfr
Copy link
Collaborator Author

PR is now rebased&fixed-up and conflict are resolved.

@mgautierfr mgautierfr force-pushed the cache_config branch 2 times, most recently from fe9ee9d to 4a1881a Compare February 12, 2025 14:38
veloman-yunkan
veloman-yunkan reviewed Feb 15, 2025
View reviewed changes
Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

All my feedback was restricted to the test code, which means that generally the PR is good to merge.

@mgautierfr
Copy link
Collaborator Author

Last push updates with your comments.
I have directly modified the commits to be ready to merge as soon as you agree (which I hope is soon)

Only test/lru_cache.cpp and test/archive.cpp files have been modified.

veloman-yunkan
veloman-yunkan reviewed Feb 17, 2025
View reviewed changes
@mgautierfr mgautierfr force-pushed the cache_config branch 2 times, most recently from 55616af to f3ea99f Compare February 17, 2025 15:59
@mgautierfr
Copy link
Collaborator Author

mgautierfr commented Feb 17, 2025 •
edited
Loading

I have added a small comment about no reset the ref_it.
Please note the test will kind of become obsolete with PR #956 which introduce a global cluster cache. So I don't it worth have a perfect test here.

@mgautierfr
Copy link
Collaborator Author

This (almost) last push rename all introduced method from snake_case to camelCase as this is our coding style. fixup commits are interleaved in the git history.

@veloman-yunkan please give a formal approval before I rebase-fixup (and reword commit messages) and merge without further approval.

veloman-yunkan
veloman-yunkan approved these changes Feb 27, 2025
View reviewed changes
Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

I only checked the full diff from the previous version without inspecting each fixup commit separately. LGTM.

@mgautierfr
Make cluster cache size configurable.
6df5d30
@mgautierfr
Make dirent cache size configurable
8677a5a
@mgautierfr
Make dirent lookup cache size configurable.
2c216e7 
- DirentLookup becoming polymorphic
- Possibility to disable lookup caching altogether
- Once created, this cache size cannot be modified
@mgautierfr
Do not use env var to limit the maximum size used by lzma.
2a62877 
Probably nobody never set this, and we don't use lzma anymore.
@mgautierfr
Remove envvalue source code.
1ec73f6 
We don't use it anymore.
@mgautierfr
Add testing of cache
e5256b5
@mgautierfr
Ensure that cache configuration is taken into account
e9c3b7b 
This make fail the test as it appears we create the dirent lookup cache
at initialization.
This will be fixed in next commits.
@mgautierfr
Move setting of UserEntry range out of mimetype.
8ce38e1 
There is no reason this setting is done while reading the mimetype.
@mgautierfr
Don't hide access to direntLookup in subfonctions in FileImpl constru…
2371453 
…ctor.
@mgautierfr
Don't create the cache dirent lookup at initialization.
4e9f37b 
Let's use a temp dirent lookup (without cache) to access
titleOrdered v1 and 'C' namespace.
@mgautierfr
Fix potential race condition on m_direntLookup initialization.
51f9e72 
Reading and writing in the same time to a unique_ptr is not thread safe.
We introduce a atomicBool to know if the unique_ptr has been initialized.

Fix #945
@mgautierfr
Correctly test that changing the cache size on live archive has effect.
8dd1508
@mgautierfr mgautierfr merged commit 2bde511 into main Feb 27, 2025
24 of 28 checks passed
@mgautierfr mgautierfr deleted the cache_config branch February 27, 2025 12:56
This was referenced Feb 27, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veloman-yunkan veloman-yunkan veloman-yunkan approved these changes

@kelson42 kelson42 Awaiting requested review from kelson42

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

Successfully merging this pull request may close these issues.

Make cache configuration available from cpp api.
2 participants
@mgautierfr @veloman-yunkan