-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
Local memoization #6
Conversation
@@ -56,6 +56,17 @@ use syn::{parse_quote, Error, Result}; | |||
/// | |||
/// Furthermore, memoized functions cannot use destructuring patterns in their | |||
/// arguments. | |||
/// | |||
/// # Local memoization |
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'd prefer to move this below the example.
/// script | ||
/// .split('+') | ||
/// .map(str::trim) | ||
/// .map(|part| match part.strip_prefix("eval ") { | ||
/// Some(path) => evaluate(&files.read(path), files), | ||
/// None => part.parse::<i32>().unwrap(), | ||
/// }) | ||
/// .sum() |
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.
And then this can just be ...
since it's the same as above.
/// // Evaluate a `.calc` script in a thread-local cache. | ||
/// // /!\ Notice the `'local` lifetime annotation. | ||
/// #[comemo::memoize(local)] | ||
/// fn evaluate(script: &'local str, files: &comemo::Tracked<'local, Files>) -> i32 { |
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.
Why is there an extra &
before the Tracked
here?
@@ -71,12 +82,26 @@ use syn::{parse_quote, Error, Result}; | |||
/// }) | |||
/// .sum() | |||
/// } | |||
/// | |||
/// // Evaluate a `.calc` script in a thread-local cache. | |||
/// // /!\ Notice the `'local` lifetime annotation. |
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.
what does /!\
do?
/// Evict the thread local cache. | ||
/// | ||
/// This removes all memoized results from the cache whose age is larger than or | ||
/// equal to `max_age`. The age of a result grows by one during each eviction | ||
/// and is reset to zero when the result produces a cache hit. Set `max_age` to | ||
/// zero to completely clear the cache. | ||
/// | ||
/// Comemo's cache is thread-local, meaning that this only evicts this thread's | ||
/// cache. |
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.
/// Evict the thread local cache. | |
/// | |
/// This removes all memoized results from the cache whose age is larger than or | |
/// equal to `max_age`. The age of a result grows by one during each eviction | |
/// and is reset to zero when the result produces a cache hit. Set `max_age` to | |
/// zero to completely clear the cache. | |
/// | |
/// Comemo's cache is thread-local, meaning that this only evicts this thread's | |
/// cache. | |
/// Evict the thread-local cache. | |
/// | |
/// This only affects functions that are annotated with `#[memoize(local)]`. |
I also noticed that the copy-pasted comment from the other function is outdated. The thread-local note can be removed from the other one.
I wonder whether the default evict should also evict the thread local cache. The overhead would be quite low if it's unused. I'm also not sure about evicting the accelerator here.
We should think about what the use case of this is vs. evict. If evict does everything, then even if you only use local comemo, evict would do the job. In which use cases does it make sense to have a second evict_local function?
/// | ||
/// Comemo's cache is thread-local, meaning that this only evicts this thread's | ||
/// cache. | ||
pub fn local_evict(max_age: usize) { |
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'd prefer the name evict_local
.
}) | ||
} }; | ||
} else { | ||
wrapped.block = parse_quote! { { |
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.
The assignment could be moved out of the if/else.
type __ARGS<'local> = <::comemo::internal::Args<#arg_ty_tuple> as ::comemo::internal::Input>::Constraint; | ||
::std::thread_local! { | ||
static __CACHE: ::comemo::internal::Cache< | ||
__ARGS<'static>, |
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.
Does this 'static
result in any restrictions in practice?
Still on your radar @Dherse ? ^^ |
Closing due to inactivity. |
Also allows local memoization of function results with
#[comemo::memoize(local)]
and a specialevict_local
function. This is specifically for interfacing with typst-preview.The only quirks is that for passing tracked and borrowed values into a locally memoized function you need to manually annotate the lifetime with
'local
specifically!I also added a test that checks local eviction, tracked in local contexts, and simple local functions.