JuliaCollections / LRUCache.jl

An implementation of an LRU Cache in Julia
Other
56 stars 23 forks source link

Allow the use `ReentrantLock` instead of `SpinLock` #39

Open gbrgr opened 1 year ago

gbrgr commented 1 year ago

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 commented 11 months ago

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

ericphanson commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

gbrgr commented 9 months ago

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