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

[TieredStorage] Have the reader until TieredStorage lazy initialized #35063

Closed

Conversation

yhchiang-sol
Copy link
Contributor

@yhchiang-sol yhchiang-sol commented Feb 3, 2024

Problem

Currently, right after TieredStorage::write_accounts() is called, it
immediately mmap the file in case it is the hot storage. It might
be more suitable to delay the mmap until the file is actually read.

Summary of Changes

Have the reader under the TieredStorage to be lazy initialized.

Test Plan

Existing unit-tests.

Dependency

#35049

Copy link

codecov bot commented Feb 3, 2024

Codecov Report

Attention: Patch coverage is 93.65079% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 81.6%. Comparing base (dd30175) to head (517ca3f).
Report is 131 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #35063   +/-   ##
=======================================
  Coverage    81.6%    81.6%           
=======================================
  Files         830      830           
  Lines      224946   224966   +20     
=======================================
+ Hits       183674   183709   +35     
+ Misses      41272    41257   -15     

Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

Also, bigger question, is wirte_accounts() here thread-safe? What happens if different threads call this method on the same instance?

@@ -49,6 +49,7 @@ pub struct TieredStorageFormat {
#[derive(Debug)]
pub struct TieredStorage {
reader: OnceLock<TieredStorageReader>,
read_only: OnceLock<bool>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like overkill here. Wdyt about using an AtomicBool instead?

Copy link
Contributor Author

@yhchiang-sol yhchiang-sol Feb 5, 2024

Choose a reason for hiding this comment

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

Sounds good. I am also thinking about using u64 (i.e., AtomicU64) that represents file size instead so that we can have a faster file_size() as well as checking whether is read-only.

@yhchiang-sol yhchiang-sol marked this pull request as draft February 8, 2024 12:38
@yhchiang-sol
Copy link
Contributor Author

Making this PR draft again as the thread-safety part has been move to a dedicate PR #35143

@github-actions github-actions bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Feb 23, 2024
@willhickey
Copy link
Contributor

This repository is no longer in use. Please re-open this pull request in the agave repo: https://github.com/anza-xyz/agave

@willhickey willhickey closed this Mar 3, 2024
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.

3 participants