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

Allow the use ReentrantLock instead of SpinLock #39

Open
gbrgr opened this issue Jul 5, 2023 · 6 comments
Open

Allow the use ReentrantLock instead of SpinLock #39

gbrgr opened this issue Jul 5, 2023 · 6 comments

Comments

@gbrgr
Copy link

gbrgr commented Jul 5, 2023

In the current implementation, if a user needs to hold the cache's lock over multiple gets/sets, one cannot use the cache's internal lock since it is a SpinLock and blocks.

Suggestion: Use a ReentrantLock or provide the user the option to use such a lock instead of a SpinLock. Moreover, implement

Base.lock(lru::LRU) = lock(lru.lock)

(likewise for unlock) to allow users taking that lock.

@ericphanson
Copy link
Member

can you explain your use-case more, why do you want to grab the lock over multiple gets/sets?

@ericphanson
Copy link
Member

ReentrantLock seems preferred in general over SpinLock reading through the julia issue tracker / PRs, so it would make sense to me to switch. But I assume @Jutho had a reason for choosing SpinLock in the first place. Maybe we can try to do some benchmarking to see what the tradeoffs are.

@Jutho
Copy link
Collaborator

Jutho commented Jan 25, 2024

I think I chose the SpinLock because it was claimed to be more performant, and I assumed that the requirements for using it were fulfilled. But I have very little experience with multithreaded code and locks and all the unexpected complications that can arise, so a ReentrantLock might be the safer solution.

I am interested in the use case here. The goal is that LRU takes care of the locking in a way that is oblivious to the user, so the user should not manually have to interfere with this. If you are trying to build a solution were you need to manually control the lock, is it even worth using LRU. Then again, I could see that you maybe want to recycle part of the implementation, so I am definitely open to this suggestion.

@gbrgr
Copy link
Author

gbrgr commented Jan 25, 2024

Please apologize the delayed reply, I missed that thread. Regarding the question as what is the use case of taking a lock over multiple gets/sets, one simple example is where one would like to implement a method which acts as get_or_new:

function get_or_new(cache::LRU{K,V}, key::K, args...)
    lock(cache) do
        if haskey(cache, key)
            cache[key]
        else
            created = V(args...)
            cache[key] = created
            return created
        end
    end
end

Now such user-level locks become easiest if one uses a ReentrantLock, as otherwise users of LRU would have to allocate their own lock, as they cannot re-lock SpinLock.

@ericphanson
Copy link
Member

I think this specific api is provided already: https://github.com/JuliaCollections/LRUCache.jl#getdefaultcallable-lrulru-key

@gbrgr
Copy link
Author

gbrgr commented Jan 25, 2024

Thanks I missed that. In any case, I think it might be worth considering changing that lock in case performance gains can be made.

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

No branches or pull requests

3 participants