-
Notifications
You must be signed in to change notification settings - Fork 215
feat: add previous() to access a tracked function's prior memoized value
#1219
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -469,6 +469,40 @@ macro_rules! setup_tracked_fn { | |
| } | ||
| } | ||
|
|
||
| /// Returns a reference to this function's memoized value from the previous | ||
| /// revision, or `None` if there is no usable previous value. | ||
| /// | ||
| /// This is only meaningful while the function is re-executing for the current | ||
| /// key: it lets the new computation build on its own prior result. Reading the | ||
| /// previous value replays that result's dependencies into the current execution, | ||
| /// so incremental validation stays correct. | ||
| /// | ||
| /// Returns `None` when the function has never completed for this key, when the | ||
| /// previous result is provisional (for example, mid-cycle), or when the memoized | ||
| /// value has been evicted (for example, due to an LRU capacity limit). | ||
| /// | ||
| /// Values accumulated by the previous execution itself are not replayed; a | ||
| /// function that accumulates must push its values again even when it returns | ||
| /// its previous result. | ||
| /// | ||
| /// For a specifiable function, the previous value may have been assigned via | ||
| /// `specify` rather than computed by this function; the two cases are | ||
| /// indistinguishable here. | ||
| /// | ||
| /// # Panics | ||
| /// | ||
| /// Panics if called outside of this tracked function's own execution. | ||
| pub fn previous<$db_lt>( | ||
| $db: &$db_lt dyn $Db, | ||
| ) -> Option<&$db_lt $output_ty> { | ||
| use ::salsa::plumbing as $zalsa; | ||
|
|
||
| $zalsa::attach($db, || { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to attach here? Is it mainly to assert the same-db constraint? |
||
| let (zalsa, zalsa_local) = $db.zalsas(); | ||
| $Configuration::fn_ingredient_($db, zalsa).previous(zalsa, zalsa_local) | ||
| }) | ||
| } | ||
|
|
||
| $zalsa::macro_if! { $is_specifiable => | ||
| pub fn specify<$db_lt>( | ||
| $db: &$db_lt dyn $Db, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ mod fetch; | |
| mod inputs; | ||
| mod maybe_changed_after; | ||
| mod memo; | ||
| mod previous; | ||
| mod specify; | ||
| mod sync; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| use crate::function::{Configuration, IngredientImpl}; | ||
| use crate::zalsa::Zalsa; | ||
| use crate::zalsa_local::ZalsaLocal; | ||
|
|
||
| impl<C> IngredientImpl<C> | ||
| where | ||
| C: Configuration, | ||
| { | ||
| /// Returns the value memoized for the active key in the previous revision, | ||
| /// replaying that result's dependencies into the active query. | ||
| /// | ||
| /// # Panics | ||
| /// | ||
| /// Panics if the active query is not an execution of this tracked function. | ||
| pub fn previous<'db>( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need to land my monomorphization PR, so that we can move this out of the |
||
| &'db self, | ||
| zalsa: &'db Zalsa, | ||
| zalsa_local: &'db ZalsaLocal, | ||
| ) -> Option<&'db C::Output<'db>> { | ||
| let Some((database_key_index, _)) = zalsa_local.active_query() else { | ||
| panic!( | ||
| "cannot access previous memoized value for {} outside of its tracked function", | ||
| C::DEBUG_NAME, | ||
| ); | ||
| }; | ||
|
|
||
| if database_key_index.ingredient_index() != self.index { | ||
| panic!( | ||
| "cannot access previous memoized value for {} while executing {database_key_index:?}", | ||
| C::DEBUG_NAME, | ||
| ); | ||
| } | ||
|
|
||
| let id = database_key_index.key_index(); | ||
| let memo_ingredient_index = self.memo_ingredient_index(zalsa, id); | ||
| let memo = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index)?; | ||
|
|
||
| if memo.header.may_be_provisional() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it's best to exclude provisional memos for now, even if they're from the previous revision (it's not guaranteed that all memos participating in a cycle are finalized). For same-revisional provisionals, we'd have to carry over the cycle heads. Ultimately, this is the same as calling self again (fixpoint!). It's a bit less clear to me what needs to happen when depending on a provisional from a previous revision. |
||
| return None; | ||
| } | ||
|
|
||
| let value = memo.value.as_ref()?; | ||
| let revisions = &memo.header.revisions; | ||
| zalsa_local.report_previous_memo_read( | ||
| revisions.durability, | ||
| revisions.origin().edges(), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I'm not sure this is sound. I have to think this through a bit more carefully. There's no guarantee that the input edges from a previous revision are still valid. That is, some input edges may no longer exist because they were garbage collected, or were never deserialized after persistent caching. Carrying these dependencies forward could also mean that a query now depends on the same IDs that only differ b y generations. |
||
| revisions.is_derived_untracked(), | ||
| revisions.tracked_struct_ids(), | ||
| #[cfg(feature = "accumulator")] | ||
| revisions.accumulated_inputs.load(), | ||
| zalsa.current_revision(), | ||
| ); | ||
| Some(value) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's unclear to me is whether this is safe if |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling out the specify case, we could recognize this and have the function return something else here (enum distinguishing the cases) but I don't think thats worth it until useful to someone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your reasoning here. The idea of specify is that it's mostly transparent. So handling them as described here makes sense to me