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

Rethink Pool design #28

Open
nickrobinson251 opened this issue Oct 2, 2023 · 2 comments
Open

Rethink Pool design #28

nickrobinson251 opened this issue Oct 2, 2023 · 2 comments

Comments

@nickrobinson251
Copy link
Member

nickrobinson251 commented Oct 2, 2023

i noticed that Pool is currently providing two things, which i think it is confusing to combine (at least in the way they're currently combined):

  1. "permits": acting as a Semaphore to limit the maximum number of objects in use at a time
  2. "store": providing a collection of objects available for re-use

But note that the limit is on the "permits" (the number of objects in use), not on the "store" (the number of object available for re-use), on which there is no limit, e.g.

julia> pool = Pool{Int}(2);

julia> for i in 1:100
           x = acquire(() -> i, pool; forcenew=true)
           release(pool, x)
       end;

julia> length(pool.values)
100

I worry this could lead to a potential memory leak for users who do not realise that the "store" can grow arbitrarily in size and never be cleaned up.

I wonder if the size of the "store" should also be limited (for example, forcenew could force an eviction either always or if the collection is at capacity)

@nickrobinson251
Copy link
Member Author

the isvalid function also has a kind of weird interaction with the stored collection on objects, because it iterated the objects and evicts them until it finds a valid one... which means which objects stay in the pool depends on their insertion order

e.g.

julia> pool = Pool{Int}(2);

julia> for i in 1:100
           x = acquire(() -> i, pool; forcenew=true)
           release(pool, x)
       end;

julia> length(pool.values)
100

julia> acquire(() -> 0, pool; isvalid=x -> x == 1);

julia> length(pool.values)
0

but if they're the other order:

julia> pool = Pool{Int}(2);

julia> for i in 100:-1:1  # <- reversed
           x = acquire(() -> i, pool; forcenew=true)
           release(pool, x)
       end;

julia> length(pool.values)
100

julia> acquire(() -> 0, pool; isvalid=x -> x == 1);


julia> length(pool.values)
99

@nickrobinson251
Copy link
Member Author

nickrobinson251 commented Oct 3, 2023

i find the "keyed" pool also a bit odd, especially the interaction with release:

  • the "permits" are not keyed
  • the "store" is keyed

when you acquire an object you must provide a key, so that we know which "store" to check, but this also creates an empty store if there isn't one already

julia> pool = Pool{String, Int}(3);

julia> pool.keyedvalues   # <- no stores
Dict{String, Vector{Int64}}()

julia> acquire(() -> 0, pool, "a");

julia> pool.keyedvalues  # <- now there is an "a" store
Dict{String, Vector{Int64}} with 1 entry:
  "a" => []

when you release an object you must provide a key, but i don't know why, if you just want to release a "permit"; you need to provide a key even though it isn't used... we check the key is of the right type for the pool but not if it's a value for which there is a store:

julia> pool = Pool{String, Int}(3);

julia> x1 = acquire(() -> 1, pool, "a");

julia> pool.cur  # <- 1 permit in use
1

julia> release(pool, x1);  # <- error to release without providing a key
ERROR: ArgumentError: invalid key `nothing` provided for pool key type String
Stacktrace:
 ...

julia> pool.cur  # <- the permit is still in use
1

julia> release(pool, :z, x1);  # <- error to release while providing a key of the wrong _type_
ERROR: ArgumentError: invalid key `z` provided for pool key type String
Stacktrace:
 ...

julia> pool.cur  # <- the permit is still in use
1

julia> release(pool, "z");  # <- fine to release while providing a unknown key 

julia> pool.cur  # <-- the permit was released
0

why do we require a key which we're not going to use, but require it to have the right type but not a known value?

if you want to release the permit and return the object to the store for re-use, then it makes sense that you need to provide a key... but we don't allow returning things to a new store (you can only make stores at acquire time - why?) and providing the wrong key type will error and not release the permit (as above), but providing an unknown key value will error but also still release the permit

julia> pool = Pool{String, Int}(3);

julia> x1 = acquire(() -> 1, pool, "a");

julia> release(pool, :z, x1)   # <- error to release / return object while providing a key of the wrong _type_
ERROR: ArgumentError: invalid key `z` provided for pool key type String
Stacktrace:
 ...

julia> pool.cur   # <- the permit is still in use
1

julia> release(pool, "z", x1)  # <- error to release /return object while providing a unknown key 
ERROR: KeyError: key "z" not found
Stacktrace:
 ...

julia> pool.cur  # <-- but the permit was released (despite the error)
0

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

1 participant