[
https://issues.apache.org/jira/browse/GEODE-6636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16875133#comment-16875133
]
Helena Bales commented on GEODE-6636:
-------------------------------------
Completing this refactor will take more effort than we initially expected. It
may be better to refactor the way the buffer pool works as a whole.
In the current implementation, a buffer is polled from the head of the queue.
It gets used if it is a big enough buffer. If it isn't big enough, it gets put
to a set. If the put fails, then we have already checked that buffer and have
therefore looped through all the buffers without finding one that is big
enough. If we have checked all the buffers and didn't find one to use, then we
create a new one.
This implementation creates a lot of garbage, and is not very thread safe.
In the implementation proposed by [this
PR|https://github.com/apache/geode/pull/3741], an iterator is created for the
buffer pool. The item at the head of the iterator is removed, and is used if it
is big enough. If it is too small, the buffer is replaced on the head of the
dequeue, and the iterator will move to point to the next item (Note: the
movement of the iterator to the next item can't be relied upon if there is
concurrent access to the dequeue, so we can't make this assumption). If we
reach the end of the dequeue without finding a buffer that is big enough, then
we will have to create one.
This implementation creates less garbage than the previous implementation, but
is no better in speed, and no more thread safe than the previous implementation.
> Buffers.acquireBuffer is not optimal
> ------------------------------------
>
> Key: GEODE-6636
> URL: https://issues.apache.org/jira/browse/GEODE-6636
> Project: Geode
> Issue Type: Improvement
> Components: core
> Reporter: Darrel Schneider
> Assignee: Helena Bales
> Priority: Major
> Labels: performance
> Time Spent: 2h 50m
> Remaining Estimate: 0h
>
> org.apache.geode.internal.net.Buffers.acquireBuffer takes buffers out of a
> ConcurrentLinkedQueue. If the buffer is too small then it adds it back to the
> queue and adds it to an IdentityHashMap. The map is just to detect if we have
> looped around and found one we added to the map in a previous iteration.
> A more efficient way to do this, which will only remove things from the queue
> that we will either throw away or use and return later, and which gets rid of
> the map, is to use ConcurrentLinkedQueue.remove(Object). You can see an
> example of this by looking at:
> AvailableConnectionManager.EqualsWithPredicate. The predicate to use with
> acquireBuffer is that the soft reference is null or that the capacity of the
> referenced buffer is large enough. If you remove one because the reference is
> null then you need to call remove(Object) again (after decrementing the
> correct stat) since all you did was find one that had been garbage collected.
> You want to keep the predicate as cheap as possible since it is called in the
> "compare-and-set" spin loop. The more you do in the predicate, the more
> likely a concurrent thread will take that buffer and you will need to spin
> around and try again.
> I was surprised when running a benchmark under the profiler to see operations
> on this IdentityHashMap show up. The benchmark was doing pr puts of all the
> same size so I would have thought all these direct buffers would be the same.
> I think it would be worth understanding what sizes of buffers will be
> requested by acquireBuffer and how often they will be acquired and returned.
> If different sizes is a normal use case then it probably would make sense to
> have more than one queue. If we could go to a queue knowing that if it has a
> buffer in it that it meets are desired size. The geode off-heap free list
> implementation does this and only the largest allocations need to search
> through a free list that has items in it that exceed a max size. Every other
> free list is found quickly be using the requested size to index an array of
> free lists, which only contain items of that size.
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
