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

Add load_if_changed to Cache to return a value only if it has changed #91

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

elichai
Copy link

@elichai elichai commented Aug 4, 2023

This is required when you need to know if you received a new value or an old one you've already seen

@vorner
Copy link
Owner

vorner commented Aug 5, 2023

Hello

I'm not saying no, but can you explain the use case of this? Conceptually, a Cache is for having an up to date value. Why would you need to know it has changed?

@elichai
Copy link
Author

elichai commented Aug 6, 2023

@vorner The use case is that I have a program with a bunch of workers that all work on the same task (highly parallelizable tasks) and I use this to swap the task if a new task comes in,
the workers need the actual T so they can mutate it while working on it, not Arc<T> so if the Arc<T> has changed I'll replace the current working task with a clone of the new T in the new Arc<T>

In other words, it looks something like that:

let mut task = watcher.get_task();
loop {
    task.id = ...;
    // work
    if let Some(new_task) = watcher.load_if_changed() {
        task = (*new_task).clone();
    }
}

@vorner
Copy link
Owner

vorner commented Aug 16, 2023

The fact you need to clone the thing inside the Arc kind of suggests arc-swap is the wrong tool for the job, to be honest. The Arc is actually getting in your way there a little bit (or at least not serving any actual purpose).

What are your performance characteristics? Is the single iteration of the work so short that the performance of the synchronization primitive actually matters? If not, RwLock<T> (or ShardedLock from crossbeam) might do the thing, or having a channel into each worker and have the sender clone the things for them (try_recv on it in the worker), or maybe do something with crossbeam::epoch. Using Arc-Swap's Cache to not use the Arc seems a bit… weird 😇

@elichai
Copy link
Author

elichai commented Aug 22, 2023

Basically what I'm trying to emulate here is a very fast watch channel,
I have workers that work on a very hot short tight loop, and after every few iterations they want to make sure they're working on the latest job, and the workers should never "wait" for a job.
So with Arc-swap I'm using a sort of atomic CoW where each worker clones the data and the data is updated by a pointer atomic swap (in a GC language this is trivially implementable, with rust it requires something like arc-swap to make sure there are no free after free or use after free)

@vorner
Copy link
Owner

vorner commented Aug 27, 2023

I think I understand what you do and somewhat of why you want to do it this way and why such API would help your case.

On the other hand, I'm still reluctant because there seem to be a lot of „this feels wrong on the design level“ smell… both for the API and the requirements… I wouldn't really feel very happy about leading other people towards similar solutions.

From your description, the right tool for your job seems to be the crossbeam::epoch, or an alternative, use something like „double-cache“ here. What I mean is:

  • Use the cache as it is, but add some kind of generation field to the job (incremented with each new job stored / wrapping_add(1)).
  • Check the generation of the job and clone as necessary. Something like:
let mut job_copy = ...;
loop {
    let central_job = cache.load();
    if central_job.generation != job_copy.generation {
        job_copy = central_job.clone();
    }
}

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.

2 participants