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

Assign memo ingredients per salsa-struct-ingredient, not globally #600

Open
nikomatsakis opened this issue Oct 16, 2024 · 3 comments · May be fixed by #614
Open

Assign memo ingredients per salsa-struct-ingredient, not globally #600

nikomatsakis opened this issue Oct 16, 2024 · 3 comments · May be fixed by #614
Labels
good first issue Good for newcomers
Milestone

Comments

@nikomatsakis
Copy link
Member

Today every tracked function gets assigned a distinct MemoIngredientIndex. These are used to index into the memo table attached to a salsa struct. But every function can be attached to salsa structs of exactly one kind, so indices only have to be distinct per salsa struct, not globally. This would save a lot of space in larger projects.

@nikomatsakis nikomatsakis added this to the Salsa 3.0 milestone Oct 16, 2024
@nikomatsakis nikomatsakis added the good first issue Good for newcomers label Oct 17, 2024
@nikomatsakis
Copy link
Member Author

Mentoring instructions

Currently 'memo ingredient indices' are a vector; the values in the vector are the fn ingredient indices that would be storing the memo (this should be documented!).

salsa/src/zalsa.rs

Lines 122 to 123 in 710691d

/// Number of memo ingredient indices created by calls to [`next_memo_ingredient_index`](`Self::next_memo_ingredient_index`)
memo_ingredients: Mutex<Vec<IngredientIndex>>,

This should be changed into a map of vectors, indexed by the "salsa struct ingredient index" (the ingredient index on which the memo is stored):

/// Map from the [`IngredientIndex`][] of a salsa struct to a list of
/// [ingredient-indices](`IngredientIndex`)for tracked functions that have this salsa struct as input.
memo_ingredient_indices: RwLock<Map<IngredientIndex, Vec<IngredientIndex>>>

then we modify next_memo_ingredient_index to take the tracked struct ingredient index as an argument

salsa/src/zalsa.rs

Lines 300 to 305 in 710691d

fn next_memo_ingredient_index(&self, ingredient_index: IngredientIndex) -> MemoIngredientIndex {
let mut memo_ingredients = self.memo_ingredients.lock();
let mi = MemoIngredientIndex(u32::try_from(memo_ingredients.len()).unwrap());
memo_ingredients.push(ingredient_index);
mi
}

This is called my the various code in salsa-macro-rules, so that will have to be modified to pass in the ingredient index for the salsa struct on which the function is invoked.

We also modify ingredient_index_for_memo similarly

salsa/src/zalsa.rs

Lines 291 to 296 in 710691d

pub(crate) fn ingredient_index_for_memo(
&self,
memo_ingredient_index: MemoIngredientIndex,
) -> IngredientIndex {
self.memo_ingredients.lock()[memo_ingredient_index.as_usize()]
}

and change the one caller of that last method:

https://github.com/salsa-rs/salsa/blob/master/src/tracked_struct.rs#L504

to pass in the self.ingredient_index.

@MichaReiser
Copy link
Contributor

@ibraheemdev you expressed interest in this task. Let me know if you start working on it so that I can assign it to you.

@davidbarsky
Copy link
Contributor

We had some interest/demand from rust-analyzer contributors to contribute to Salsa and @ShoyuVanilla expressed interested in this work. I also believe that he has bandwidth to work on this sooner than @ibraheemdev and memory usage is a concern for rust-analyzer, so for those reasons, I'd like to assign him to this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants