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

Reuse Nodes in Pools #19

Closed
wants to merge 5 commits into from
Closed

Reuse Nodes in Pools #19

wants to merge 5 commits into from

Conversation

Drvi
Copy link
Member

@Drvi Drvi commented Apr 21, 2023

This is an idea how to save us allocations for new node in release. Pools now carry a vector of Nodes which can be reused. push!ing to a Stack now also accepts an empty Node as a new argument and we introduce a _popnode! operation that implements pop! for the Stack and also yields a buffered Node from the Pool. This does seem to reduce allocations but is probably not faster (at least it doesn't seem to be from my small experiments)... Would be great to have a proper benchmark we care about to evaluate optimizations like this.

quinnj and others added 3 commits April 20, 2023 08:47
Also split the now-large ConcurrentUtilities.jl file into more
manageable individual files.
@Drvi Drvi requested a review from quinnj April 21, 2023 16:46
src/concurrentstack.jl Outdated Show resolved Hide resolved
src/concurrentstack.jl Outdated Show resolved Hide resolved
@@ -57,13 +75,19 @@ function Base.acquire(f, pool::Pool{K, T}, key=nothing; forcenew::Bool=false, is
forcenew && return f()
# otherwise, check if there's an existing object in the pool to reuse
objs = Base.@lock pool.values get!(() -> ConcurrentStack{T}(), pool.values[], key)
obj = pop!(objs)
while obj !== nothing
prev_node = node = _popnode!(objs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this prev_node for?

@quinnj quinnj force-pushed the jq-pools branch 2 times, most recently from fb01d42 to 24a00b8 Compare April 21, 2023 18:45
@quinnj
Copy link
Member

quinnj commented Apr 21, 2023

Refactored the pool to not use concurrentstack: #18

@quinnj quinnj closed this Apr 21, 2023
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