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 #614

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ShoyuVanilla
Copy link
Contributor

Closes #600

Copy link

netlify bot commented Nov 13, 2024

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 38ad655
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/673d39260520da0008776f8d

Copy link

codspeed-hq bot commented Nov 13, 2024

CodSpeed Performance Report

Merging #614 will not alter performance

Comparing ShoyuVanilla:issue-600 (38ad655) with master (e4d36da)

Summary

✅ 8 untouched benchmarks

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>>,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would be inclined to add a method ingredient_index to SalsaStructInDb but best to hear what @nikomatsakis thinks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to get the IngredientIndex for salsa struct here;

impl $zalsa::Jar for $Configuration {
fn create_ingredients(
&self,
aux: &dyn $zalsa::JarAux,
first_index: $zalsa::IngredientIndex,
) -> Vec<Box<dyn $zalsa::Ingredient>> {
let fn_ingredient = <$zalsa::function::IngredientImpl<$Configuration>>::new(
first_index,
aux,
);

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.

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

I don't think we currently have a test for a tracked fn that has more than one argument (other than db) and thus uses the interner. Could you double check if that's indeed the case and, if so, add a test for it?

src/zalsa.rs Outdated
Comment on lines 123 to 125
/// [ingredient-indices](`IngredientIndex`)for tracked functions that have this salsa struct
/// as input.
memo_ingredients: RwLock<FxHashMap<IngredientIndex, Vec<IngredientIndex>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
/// [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
Comment on lines 123 to 125
/// [ingredient-indices](`IngredientIndex`)for tracked functions that have this salsa struct
/// as input.
memo_ingredients: RwLock<FxHashMap<IngredientIndex, Vec<IngredientIndex>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@ShoyuVanilla
Copy link
Contributor Author

I don't think we currently have a test for a tracked fn that has more than one argument (other than db) and thus uses the internet. Could you double check if that's indeed the case and, if so, add a test for it?

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>);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed salsa struct ingredient lookup as I said here but I had to change this to avoid deadlocks 😢

});
if should_create {
let aux = JarAuxImpl(self, &jar_map);
let ingredients = jar.create_ingredients(&aux, index);
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a comment on jar_map that suggests that it's important to only access ingredients_vec while the lock on jar_map is held

https://github.com/MichaReiser/salsa/blob/38ad6555ebe4374bbc69022e005f4136a1839ad9/src/zalsa.rs#L127-L133

So moving it out to avoid the deadlock might not be entirely safe

Copy link
Contributor

Choose a reason for hiding this comment

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

For example. What could happen now is that two calls end up with the same index because the new ingredient isn't pushed on the ingredients_vec until line 206, so that self.ingredients_vec.len() returns the same length.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll fix this

@nikomatsakis
Copy link
Member

@ShoyuVanilla hey, cool! My apologies for radio silence. I've been overwhelmed. I'm going to carve out more time for salsa though so let me take a look...

Copy link
Member

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

This looks good! Took me a bit to get my head in the right space. I left comments, can you check that they are accurate and merge them in? After that, I'd say we can land this.

.or_insert_with(|| {
let index = IngredientIndex::from(self.ingredients_vec.len());
let ingredients = jar.create_ingredients(self, index);
let mut should_create = false;
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite follow this -- why move this logic out from the or_insert_with call?

Copy link
Member

Choose a reason for hiding this comment

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

I guess it is so that ingredient construction can call lookup_jar_by_type?

Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to have a comment here explaining that interaction

}

pub trait JarAux {
fn next_memo_ingredient_index(&self, ingredient_index: IngredientIndex) -> MemoIngredientIndex;
fn lookup_jar_by_type(&self, jar: &dyn Jar) -> Option<IngredientIndex>;
Copy link
Member

Choose a reason for hiding this comment

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

Pre-existing, but we should add comments to these methods. What is the reason people would invoke lookup_jar_by_type and next_memo_ingredient_index?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fn lookup_jar_by_type(&self, jar: &dyn Jar) -> Option<IngredientIndex>;
/// Return index of first ingredient from `jar` (based on the dynamic type of `jar`).
/// Returns `None` if the jar has not yet been added.
/// Used by tracked functions to lookup the ingredient index for the salsa struct they take as argument.
fn lookup_jar_by_type(&self, jar: &dyn Jar) -> Option<IngredientIndex>;

}

#[salsa::tracked]
fn tracked_fn<'db>(db: &'db dyn salsa::Database, input: MyInput, interned: MyInterned<'db>) -> u32 {
Copy link
Member

Choose a reason for hiding this comment

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

Why this particular test? Tracked functions with multiple arguments today compile to a tracked function on a hidden interned value -- is that what you were intending to test?

@@ -199,7 +202,19 @@ macro_rules! setup_tracked_fn {
aux: &dyn $zalsa::JarAux,
first_index: $zalsa::IngredientIndex,
) -> Vec<Box<dyn $zalsa::Ingredient>> {
let struct_index = $zalsa::macro_if! {
if $needs_interner {
first_index.successor(0)
Copy link
Member

Choose a reason for hiding this comment

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

I...guess this is fine. It's a bit surprising to use the index of something that hasn't been made yet. An alternative would be to flip the order so that we create the interned struct first and then pass its index to the function. There'd probably be some minor adjustments made elsewhere in the file as a result but shouldn't be too bad.

if $needs_interner {
first_index.successor(0)
} else {
<$InternedData as $zalsa::SalsaStructInDb>::lookup_ingredient_index(aux)
Copy link
Member

Choose a reason for hiding this comment

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

OK, I see now the role of lookup_ingredient_index.

first_index.successor(0)
} else {
<$InternedData as $zalsa::SalsaStructInDb>::lookup_ingredient_index(aux)
.expect(
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 1000% percent sure that the struct ingredient will always have been created first. If this code doesn't run until the tracked functon is first called, I suppose that's true, but if it could run at some other time maybe not? Still, this seems like a good assertion for now to simplify our lives. If it's not true we can probably add some kind of dependency mechanism to force it to be true.

@@ -23,10 +23,19 @@ pub trait Jar: Any {
aux: &dyn JarAux,
first_index: IngredientIndex,
) -> Vec<Box<dyn Ingredient>>;

/// If this jar's first ingredient is a salsa struct, return its `TypeId`
fn salsa_struct_type_id(&self) -> Option<TypeId>;
}

pub trait JarAux {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub trait JarAux {
/// Methods on the Salsa database available to jars while they are creating their ingredients.
pub trait JarAux {

fn next_memo_ingredient_index(&self, ingredient_index: IngredientIndex) -> MemoIngredientIndex;
fn lookup_jar_by_type(&self, jar: &dyn Jar) -> Option<IngredientIndex>;

fn next_memo_ingredient_index(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fn next_memo_ingredient_index(
/// Returns the memo ingredient index that should be used to attach data from the given tracked function
/// to the given salsa struct (which the fn accepts as argument).
///
/// The memo ingredient indices for a given function must be distinct from the memo indices
/// of all other functions that take the same salsa struct.
///
/// # Parameters
///
/// * `struct_ingredient_index`, the index of the salsa struct the memo will be attached to
/// * `ingredient_index`, the index of the tracked function whose data is stored in the memo
fn next_memo_ingredient_index(

let index = IngredientIndex::from(self.ingredients_vec.len());
let ingredients = jar.create_ingredients(self, index);
let mut should_create = false;
let index = *jar_map.entry(jar_type_id).or_insert_with(|| {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let index = *jar_map.entry(jar_type_id).or_insert_with(|| {
// First record the index we will use into the map and then go and create the ingredients.
// Those ingredients may invoke methods on the `JarAux` trait that read from this map
// to lookup ingredient indices for already created jars.
//
// Note that we still hold the lock above so only one jar is being created at a time and hence
// ingredient indices cannot overlap.
let index = *jar_map.entry(jar_type_id).or_insert_with(|| {

@@ -186,21 +188,22 @@ impl Zalsa {
{
let jar_type_id = jar.type_id();
let mut jar_map = self.jar_map.lock();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let mut jar_map = self.jar_map.lock();
// Important: we hold the lock on `jar_map` during ingredient construction to
// ensure all ingredients are created atomically.
let mut jar_map = self.jar_map.lock();

@ShoyuVanilla
Copy link
Contributor Author

This looks good! Took me a bit to get my head in the right space. I left comments, can you check that they are accurate and merge them in? After that, I'd say we can land this.

Thanks for the review! I'll fix those things by this weekend

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.

Assign memo ingredients per salsa-struct-ingredient, not globally
3 participants