Assign memo ingredients per salsa-struct-ingredient #614
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #614 will not alter performanceComparing Summary
|
src/zalsa.rs
Outdated
| @@ -130,6 +132,9 @@ pub struct Zalsa { | |||
| /// adding new kinds of ingredients. | |||
| jar_map: Mutex<FxHashMap<TypeId, IngredientIndex>>, | |||
|
|
|||
| /// Map from the type-id of a salsa struct to the index of its first ingredient. | |||
| salsa_struct_map: Mutex<FxHashMap<TypeId, IngredientIndex>>, | |||
|
|
|||
There was a problem hiding this comment.
I don't like this new table for SalsaStruct -> IngredientIndex but I couldn't find any other way to pass the salsa-struct-ingredient to its tracked function when creating the function's ingredient, as salsa-struct-ingredient is accessible only with its Configuration, that is private inside the macro-expanded code of that salsa struct.
Would there be some good alternatives?
There was a problem hiding this comment.
I would be inclined to add a method ingredient_index to SalsaStructInDb but best to hear what @nikomatsakis thinks
There was a problem hiding this comment.
We need to get the IngredientIndex for salsa struct here;
salsa/components/salsa-macro-rules/src/setup_tracked_fn.rs
Lines 196 to 205 in e4d36da
and we can't get the actual instance of salsa struct here. All we have is JarAux - actually, this is Zalsa -, so that function should be a static method, I guess. (I'm afraid that this makes SalsaStructInDb dyn-incompatible but we are not using it as a trait object anywhere in salsa)
Thus, the function would be implemented inside our setup_* macros like;
fn ingredient_index(aux: &dyn JarAux) -> IngredientIndex {
aux.lookup_jar_by_type(JarImpl::<$Configuration>::default())
}and we need to expose some method like lookup_jar_by_type in JarAux, I think.
src/zalsa.rs
Outdated
| /// [ingredient-indices](`IngredientIndex`)for tracked functions that have this salsa struct | ||
| /// as input. | ||
| memo_ingredients: RwLock<FxHashMap<IngredientIndex, Vec<IngredientIndex>>>, |
There was a problem hiding this comment.
Nit:
| /// [ingredient-indices](`IngredientIndex`)for tracked functions that have this salsa struct | |
| /// as input. | |
| memo_ingredients: RwLock<FxHashMap<IngredientIndex, Vec<IngredientIndex>>>, | |
| /// [ingredient-indices](`IngredientIndex`) for tracked functions that have this salsa struct | |
| /// as input. | |
| memo_ingredient_indices: RwLock<FxHashMap<IngredientIndex, Vec<IngredientIndex>>>, |
I also have a slight preference to the name @nikomatsakis suggest in the mentoring instructions (memo_ingredient_indices)
src/zalsa.rs
Outdated
| /// [ingredient-indices](`IngredientIndex`)for tracked functions that have this salsa struct | ||
| /// as input. | ||
| memo_ingredients: RwLock<FxHashMap<IngredientIndex, Vec<IngredientIndex>>>, |
There was a problem hiding this comment.
We could consider using a Vec<Vec<IngredientIndex>> here and directly index into the Vec using the IngredientIndex as an offset. This would result in wasting some memory because not every IngredientIndex is an index for a tracked struct but it would avoid one additional hashing step
Sure! I'll add tests and edit things you commented when I get home. Thanks for the review |
| impl JarAux for Zalsa { | ||
| fn next_memo_ingredient_index(&self, ingredient_index: IngredientIndex) -> MemoIngredientIndex { | ||
| let mut memo_ingredients = self.memo_ingredients.lock(); | ||
| struct JarAuxImpl<'a>(&'a Zalsa, &'a FxHashMap<TypeId, IngredientIndex>); |
There was a problem hiding this comment.
I changed salsa struct ingredient lookup as I said here but I had to change this to avoid deadlocks 😢
Closes #600