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

Local memoization #6

Closed
wants to merge 1 commit into from
Closed

Local memoization #6

wants to merge 1 commit into from

Conversation

Dherse
Copy link
Contributor

@Dherse Dherse commented Dec 29, 2023

Also allows local memoization of function results with #[comemo::memoize(local)] and a special evict_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!

#[comemo::memoize(local)]
fn dump<'local>(mut sink: TrackedMut<'local, Emitter>) {
    sink.emit("a");
    sink.emit("b");
    let c = sink.len_or_ten().to_string();
    sink.emit(&c);
}

I also added a test that checks local eviction, tracked in local contexts, and simple local functions.

@@ -56,6 +56,17 @@ use syn::{parse_quote, Error, Result};
///
/// Furthermore, memoized functions cannot use destructuring patterns in their
/// arguments.
///
/// # Local memoization
Copy link
Member

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.

Comment on lines +90 to +97
/// 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()
Copy link
Member

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 {
Copy link
Member

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.
Copy link
Member

Choose a reason for hiding this comment

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

what does /!\ do?

Comment on lines +99 to +107
/// 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.
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
/// 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) {
Copy link
Member

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! { {
Copy link
Member

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>,
Copy link
Member

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?

@astrale-sharp
Copy link

Still on your radar @Dherse ? ^^

@laurmaedje
Copy link
Member

Closing due to inactivity.

@laurmaedje laurmaedje closed this Apr 16, 2024
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.

3 participants