jaemk / cached

Rust cache structures and easy function memoization
MIT License
1.57k stars 95 forks source link

sync_writes isn't working correctly when different values for function parameters are used #158

Open 0xForerunner opened 1 year ago

0xForerunner commented 1 year ago

Here is my fn

#[cached(result = true, time = 0, size = 500, sync_writes = true)]
pub async fn token_info_cache(addr: String) -> ApiResult<(Json<TokenInfoResponse>, String)> {
    let msg = cw20_base::msg::QueryMsg::TokenInfo {};
    info!("   >> Fetching token_info for {}", addr);
    let string = refresh(addr, msg).await?;
    let value: TokenInfoResponse = serde_json::from_str(&string)?;
    Ok((Json::from(value), string))
}

calling concurrently with different values is causing the fn to execute sequentially which is not desired. Ex.

let (a, b) = join!(
    token_info_cache("a".to_string()),
    token_info_cache("b".to_string()),
)

This will run sequentially and not concurrently. Removing sync_writes = true fixes the issue and it runs concurrently but obviously doesn't sync writes when the argument is the same. This is using v0.44.0

jaemk commented 1 year ago

This is currently working as intended, but I understand the desire to only synchronize on a per key basis. Support for this could be added (preferably behind another macro arg like sync_value_writes = true) by updating the macro to wrap the individual cached values in another mutex. That would come with the caveat of requiring two locks for every read

0xForerunner commented 1 year ago

If you go here the docs say this:

use cached::proc_macro::cached;

/// Cache an optional function. Only `Some` results are cached.
/// When called concurrently, duplicate argument-calls will be
/// synchronized so as to only run once - the remaining concurrent
/// calls return a cached value.
#[cached(size=1, option = true, sync_writes = true)]
fn keyed(a: String) -> Option<usize> {
    if a == "a" {
        Some(a.len())
    } else {
        None
    }
}

Which would indicate that it should function as I am describing no? But yes I see your point about requiring two locks...

jaemk commented 1 year ago

Yes, sorry the docs are incorrect!

0xForerunner commented 1 year ago

Ah no problem at all! P.S. I love this crate, it's an absolute joy to use. I do think I would find the sync_value_writes = true feature very useful. For my use case I don't think having two locks would meaningfully impact performance.

jaemk commented 1 year ago

The effect of that option is here: when true, the lock is just held for the function execution instead of being released for others to read https://github.com/jaemk/cached/blob/f1bcfd6e7611e817c2c7f95a0fafc002f38880a6/cached_proc_macro/src/cached.rs#L236