Skip to content

Optionally force invalidation on cache eviction #853

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions components/salsa-macro-rules/src/setup_tracked_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ macro_rules! setup_tracked_fn {

assert_return_type_is_update: {$($assert_return_type_is_update:tt)*},

force_invalidation_on_cache_eviction: $force_invalidation_on_cache_eviction:literal,

// Annoyingly macro-rules hygiene does not extend to items defined in the macro.
// We have the procedural macro generate names for those items that are
// not used elsewhere in the user's code.
Expand Down Expand Up @@ -192,6 +194,7 @@ macro_rules! setup_tracked_fn {
line: line!(),
};
const DEBUG_NAME: &'static str = stringify!($fn_name);
const FORCE_INVALIDATION_ON_CACHE_EVICTION: bool = $force_invalidation_on_cache_eviction;

type DbView = dyn $Db;

Expand Down
1 change: 1 addition & 0 deletions components/salsa-macros/src/accumulator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ impl AllowedOptions for Accumulator {
const LRU: bool = false;
const CONSTRUCTOR_NAME: bool = false;
const ID: bool = false;
const FORCE_INVALIDATION_ON_CACHE_EVICTION: bool = false;
}

struct StructMacro {
Expand Down
2 changes: 2 additions & 0 deletions components/salsa-macros/src/input.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ impl crate::options::AllowedOptions for InputStruct {
const CONSTRUCTOR_NAME: bool = true;

const ID: bool = false;

const FORCE_INVALIDATION_ON_CACHE_EVICTION: bool = false;
}

impl SalsaStructAllowedOptions for InputStruct {
Expand Down
2 changes: 2 additions & 0 deletions components/salsa-macros/src/interned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ impl crate::options::AllowedOptions for InternedStruct {
const CONSTRUCTOR_NAME: bool = true;

const ID: bool = true;

const FORCE_INVALIDATION_ON_CACHE_EVICTION: bool = false;
}

impl SalsaStructAllowedOptions for InternedStruct {
Expand Down
18 changes: 18 additions & 0 deletions components/salsa-macros/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ pub(crate) struct Options<A: AllowedOptions> {
/// If this is `Some`, the value is the `<ident>`.
pub id: Option<syn::Path>,

pub force_invalidation_on_cache_eviction: Option<syn::Ident>,

/// Remember the `A` parameter, which plays no role after parsing.
phantom: PhantomData<A>,
}
Expand All @@ -112,6 +114,7 @@ impl<A: AllowedOptions> Default for Options<A> {
lru: Default::default(),
singleton: Default::default(),
id: Default::default(),
force_invalidation_on_cache_eviction: Default::default(),
}
}
}
Expand All @@ -133,6 +136,7 @@ pub(crate) trait AllowedOptions {
const LRU: bool;
const CONSTRUCTOR_NAME: bool;
const ID: bool;
const FORCE_INVALIDATION_ON_CACHE_EVICTION: bool;
}

type Equals = syn::Token![=];
Expand Down Expand Up @@ -351,6 +355,20 @@ impl<A: AllowedOptions> syn::parse::Parse for Options<A> {
"`id` option not allowed here",
));
}
} else if ident == "force_invalidation_on_cache_eviction" {
if A::FORCE_INVALIDATION_ON_CACHE_EVICTION {
if let Some(old) = options.force_invalidation_on_cache_eviction.replace(ident) {
return Err(syn::Error::new(
old.span(),
"option `force_invalidation_on_cache_eviction` provided twice",
));
}
} else {
return Err(syn::Error::new(
ident.span(),
"`force_invalidation_on_cache_eviction` option not allowed here",
));
}
} else {
return Err(syn::Error::new(
ident.span(),
Expand Down
5 changes: 5 additions & 0 deletions components/salsa-macros/src/tracked_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ impl crate::options::AllowedOptions for TrackedFn {
const CONSTRUCTOR_NAME: bool = false;

const ID: bool = false;

const FORCE_INVALIDATION_ON_CACHE_EVICTION: bool = true;
}

struct Macro {
Expand Down Expand Up @@ -147,6 +149,8 @@ impl Macro {
let lru = Literal::usize_unsuffixed(self.args.lru.unwrap_or(0));

let return_ref: bool = self.args.return_ref.is_some();
let force_invalidation_on_cache_eviction: bool =
self.args.force_invalidation_on_cache_eviction.is_some();

// The path expression is responsible for emitting the primary span in the diagnostic we
// want, so by uniformly using `output_ty.span()` we ensure that the diagnostic is emitted
Expand Down Expand Up @@ -185,6 +189,7 @@ impl Macro {
lru: #lru,
return_ref: #return_ref,
assert_return_type_is_update: { #assert_return_type_is_update },
force_invalidation_on_cache_eviction: #force_invalidation_on_cache_eviction,
unused_names: [
#zalsa,
#Configuration,
Expand Down
2 changes: 2 additions & 0 deletions components/salsa-macros/src/tracked_struct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ impl crate::options::AllowedOptions for TrackedStruct {
const CONSTRUCTOR_NAME: bool = true;

const ID: bool = false;

const FORCE_INVALIDATION_ON_CACHE_EVICTION: bool = false;
}

impl SalsaStructAllowedOptions for TrackedStruct {
Expand Down
1 change: 1 addition & 0 deletions src/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ pub type Memo<C> = memo::Memo<<C as Configuration>::Output<'static>>;
pub trait Configuration: Any {
const DEBUG_NAME: &'static str;
const LOCATION: crate::ingredient::Location;
const FORCE_INVALIDATION_ON_CACHE_EVICTION: bool;

/// The database that this function is associated with.
type DbView: ?Sized + crate::Database;
Expand Down
9 changes: 7 additions & 2 deletions src/function/execute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::function::{Configuration, IngredientImpl};
use crate::loom::sync::atomic::{AtomicBool, Ordering};
use crate::zalsa::{MemoIngredientIndex, Zalsa, ZalsaDatabase};
use crate::zalsa_local::{ActiveQueryGuard, QueryRevisions};
use crate::{Event, EventKind, Id, Revision};
use crate::{Durability, Event, EventKind, Id, Revision};

impl<C> IngredientImpl<C>
where
Expand Down Expand Up @@ -269,7 +269,12 @@ where
// Query was not previously executed, or value is potentially
// stale, or value is absent. Let's execute!
let new_value = C::execute(db, C::id_to_input(db, id));
let mut revisions = active_query.pop();

(new_value, active_query.pop())
if C::FORCE_INVALIDATION_ON_CACHE_EVICTION {
revisions.durability = Durability::MIN;
}

(new_value, revisions)
}
}
4 changes: 4 additions & 0 deletions src/function/maybe_changed_after.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ where
return VerifyResult::Changed;
};

if C::FORCE_INVALIDATION_ON_CACHE_EVICTION && memo.value.is_none() {
return VerifyResult::Changed;
}

let can_shallow_update = self.shallow_verify_memo(zalsa, database_key_index, memo);
if can_shallow_update.yes() && !memo.may_be_provisional() {
self.update_shallow(zalsa, database_key_index, memo, can_shallow_update);
Expand Down
61 changes: 61 additions & 0 deletions tests/force_invalidation_on_cache_eviction.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
//! Tests that `force_invalidation_on_cache_eviction` causes dependent queries
//! to be recomputed if our memo is missing, even if our result is unchanged.

mod common;
use common::LogDatabase;
use expect_test::expect;
use salsa::{Durability, Setter};
use test_log::test;

#[salsa::input]
struct MyInput {
field: (),
}

#[salsa::tracked(force_invalidation_on_cache_eviction, lru = 1)]
fn intermediate_result(db: &dyn LogDatabase, input: MyInput) {
db.push_log(format!("intermediate_result({})", input.0.as_u32()));
input.field(db);
}

#[salsa::tracked]
fn final_result(db: &dyn LogDatabase, input: MyInput) {
db.push_log("final_result".to_string());
intermediate_result(db, input);
}

#[test]
fn execute() {
let mut db = common::LoggerDatabase::default();

let high = MyInput::builder(()).durability(Durability::HIGH).new(&db);
let low = MyInput::new(&db, ());

final_result(&db, high);
// on first run, both intermediate and final results were computed
db.assert_logs(expect![[r#"
[
"final_result",
"intermediate_result(0)",
]"#]]);

// an intermediate result for a different input will evict the original memo
// from the cache (because query's lru = 1) when revision is bumped
intermediate_result(&db, low);
db.assert_logs(expect![[r#"
[
"intermediate_result(1)",
]"#]]);

// bump the revision, causing cache eviction
low.set_field(&mut db).to(());

final_result(&db, high);
// now, despite unchanged intermediate result, final result was recomputed
// (because intermediate query is `force_invalidation_on_cache_eviction`)
db.assert_logs(expect![[r#"
[
"final_result",
"intermediate_result(0)",
]"#]]);
}