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

Refactor ExistanceStore #418

Merged
merged 1 commit into from Nov 27, 2023
Merged

Refactor ExistanceStore #418

merged 1 commit into from Nov 27, 2023

Conversation

allada
Copy link
Collaborator

@allada allada commented Nov 26, 2023

Refactor Existance Store to be compatible with store API.


This change is Reviewable

Copy link
Collaborator Author

@allada allada left a comment

Choose a reason for hiding this comment

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

+@blakehatch

Reviewable status: 0 of 10 files reviewed, all discussions resolved (waiting on @blakehatch)

Copy link
Contributor

@blakehatch blakehatch left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @allada)


native-link-store/src/existence_store.rs line 88 at r1 (raw file):

        self.pin_inner()
            .has_with_results(&not_cached_digests, &mut inner_results)
            .await?;

Nit: .err_tip(|| "In ExistenceStore::inner_has_with_results")


native-link-util/src/evicting_map.rs line 33 at r1 (raw file):

#[derive(Serialize, Deserialize, PartialEq, Debug, Clone)]
pub struct SerializedLRU {
    pub data: Vec<(DigestInfo, i32)>,

Nit: Is there a way to use evicting_map for it's pruning capabilities without having to store any data as existence is just being checked? Probably an over-optimization especially since this approach limits the size of the data stored and by extension the upside of an optimization like this but something to consider.

Copy link
Collaborator Author

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @allada)


native-link-util/src/evicting_map.rs line 33 at r1 (raw file):

Previously, blakehatch (Blake Hatch) wrote…

Nit: Is there a way to use evicting_map for it's pruning capabilities without having to store any data as existence is just being checked? Probably an over-optimization especially since this approach limits the size of the data stored and by extension the upside of an optimization like this but something to consider.

Storing a () (ie: void) costs 1 byte (in C, so probably the same in rust), so the overhead is so small it is negligible.

Long answer: Yes, but it would be a pain to refactor and use.

Refactor Existance Store to be compatible with store API.
Copy link
Collaborator Author

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 10 files reviewed, all discussions resolved (waiting on @blakehatch)


native-link-store/src/existence_store.rs line 88 at r1 (raw file):

Previously, blakehatch (Blake Hatch) wrote…

Nit: .err_tip(|| "In ExistenceStore::inner_has_with_results")

Done.

Copy link
Contributor

@blakehatch blakehatch left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @allada)

@blakehatch blakehatch merged commit 08b2954 into main Nov 27, 2023
13 of 15 checks passed
@blakehatch blakehatch deleted the fix-existance-store branch November 27, 2023 01:14
TripleKai pushed a commit to TripleKai/turbo-cache that referenced this pull request Nov 29, 2023
Refactor Existance Store to be compatible with store API.
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.

None yet

2 participants