Pool Wrapper with Queue

[gregw]

For #9311
This adds a QueuedPool that can wrap any other pool and provided a queue of Entries as a kind of cache.
This is an alternative to #9319

[gregw]

@lorban do you want me to polish this whilst you work on benchmarks, or do you wish to take over the branch?
@lorban @sbordet thoughts on this approach vs #9319 ? Should we just close the other one and only resurrect it if this approach doesn't work for some reason?

[lorban]

@gregw Benchmarks should be working again by tomorrow, in which case taking over this branch is my next step. If I'm late restoring benchmarks, I'll let you know.

I find this approach superior to #9319 so IMHO the latter can be closed unless we find a showstopper on the way.

[lorban]

I've introduced two Pool implementations:

• QueuedPool which is based on a ConcurrentLinkedQueue, takes some liberties with the Pool contract and cannot multiplex.
• CompoundPool which wraps ConcurrentPool and QueuedPool, using a heuristic to figure out the ConcurrentPool size. It also cannot multiplex.

ArrayByteBufferPool simply uses CompoundPool instead of ConcurrentPool, and MultiPartServletTest.testLargePart() now "only" is ~30% slower with ArrayByteBufferPool vs ByteBufferPool.NonPooling which IMHO is reasonable.

I still believe tackling this problem at the Pool level is the right way, but I'm much less convinced about the details (3 Pool impl with one wrapping the other 2, naming, the fact that QueuedPool doesn't strictly follow the Pool contract...).

[gregw]

I think the basic approach shows promise and I don't think we should hold up a merge too much considering how pathological the current pool can be.

It would be nice to see how we could configure some alternative compound pools in XML.

[gregw]

I'd rather a constructor that just takes 2 other pools as that will allow the full configuration options to be available. Perhaps have some static builder methods that can make some common combinations, but we shouldn't bake only one combo into the class.

[lorban]

I considered such constructor, but how to make sure both underlying pools are compatible, especially w.r.t multiplexing?

[gregw]

I still think this is the right thing to do, even if we cannot have isMultiplexing.
IT would be better even if the constructor was private an only a static factory method with this signature was available

[gregw]

We need someway of checking that both pools have compatible multiplexing capabilities. Do we need to be able to ask a Pool what it's max multiple is? Or perhaps just ask it if it has a non unary max multiple function in a isMultiplexing method. If one pool is multiplexing, then the other one should be as well.

[lorban]

That's a can of worms. I considered adding a isMultiplexing method to Pool but then I realized that multiplexing is based on a constructor-provided ToIntFunction which makes it impossible to be 100% certain both pools are going to use the same, or compatible multiplexing functions.

The best I can think of is to have such isMultiplexing method in Pool and make ConcurrentPool return false only when the constructor that doesn't take a ToIntFunction has been used.

[gregw]

that works for me.

[sbordet]

But does not work, as I can pass a ToIntFunction that returns 1.
I think this class is better as a private inner class of ArrayByteBufferPool, as it's not really reusable that much.

[gregw]

If we introduce another pool that can handle queued multiplexed pools, then it will get more usage. It is a util class and I see no problem exposing it. All util classes can be used badly. If we provide some static factory methods that easily build combinations that work well together, then that will encourage good usage. I am OK with a protected constructor an only public static factory methods. Then if somebody really wanted to do something strange, they could extend.

[gregw]

Would it be possible to have a multiplexing TreeSetPool? Pool Entries would be stored in a TreeSet that sorted on the remaining multiplex capability of each entry. Thus an acquire would take the most allocated entry (until it had no capacity left and was taken out of the set). Perhaps a TreeSet is not the right data structure as it is hard to remove and then re-add, but some sorted queue could work.
We would need this only if, for example, in the client we have need of more that 256 multiplexed connections to the same destination. That is plausible!

[gregw]

Perhaps a PriorityQueue could be used. Entries would be taken from the queue on every acquire, have their multiplex count incremented and then returned to the queue to be re-sorted. On release, we need to be able also remove and re-add the entry, so the remove(Object) operation on the queue needs to be good... which it might be as there is a heap underneath it? Besides, for the multiple use case of connection pools, it does not need to by hyper efficient.

[lorban]

@gregw finding a sorted and concurrent structure with adequate perf to hold the multiplexed entries sounds challenging. I do not think we currently have a good use-case for it, as only H2 connections in the client currently are pooled and using the multiplexing support. But maybe we just found a good reason to not warn when a too large ConcurrentPool is configured?

[gregw]

It should be fairly simple to find a O(log(n)) sorted queue that will outperform an O(N) search for a connection with remaining capacity. For these kind of use-cases, we are not looking for stunning performance, we just want to avoid the lack of graceful degradation we currently see. Not sucking will be good enough for now.

[gregw]

@sbordet, as this was originally my PR, I can't review. I'm OK with it in it's current form (other than my niggle about a better constructor).

[sbordet]

I'm not convinced that CompoundPool and QueuedPool should be public in jetty-util, as they are ok for ABBP, but that's basically it.

Also, I am worried about ConnectionPool performance too now, so I would like to see a more generic solution that supports multiplexing.

[sbordet]

Not a biggie, but I'd rename this to OPTIMAL_MAX_SIZE, or RECOMMENDED_MAX_SIZE as "max" refers to the size so they should be closer.

[sbordet]

WIth such as low value, I am now worried about ConnectionPool having a bad performance, in particular for load testing.

[gregw]

Then let's add a priority queue implementation that will sort entries by multiplexed availability. It should not be too hard to write and may even be OK as the primary pool for the connections, as the concurrent pool has to use a poor strategy to work with multiplexing anyway.

[sbordet]

But does not work, as I can pass a ToIntFunction that returns 1.
I think this class is better as a private inner class of ArrayByteBufferPool, as it's not really reusable that much.

[sbordet]

I don't like this class being public in jetty-util, but have the big restriction of not being multiplexed.
For example I would not be able to use it for ConnectionPool.

I think this implementation should be private to ArrayByteBufferPool, but we do need an implementation that supports multiplexing for ConnectionPool.

[gregw]

I don't think we need to make it private. It is a perfectly good pool implementation that just happens not to take a multiplex int function, so it can't do multiplexing.

[lorban]

I've renamed the CompoundPool to ConcurrentOverflowingToQueuedPool and moved it closer to ArrayByteBufferPool as it specifically designed to solve that issue and it's fairly trivial.

QueuedPool can stay in jetty-util IMHO as it's usable the way it is, even if it does not support multiplexing. We can add an extra Pool implementation that is both queue-based and supports multiplexing.

[gregw]

@lorban I'm sorry but ConcurrentOverflowingToQueuedPool is a silly name. The only thing about that class which that name applies to is the constructor. Everything else about that class is just CompoundPool.

We should name the class for what it does, not for hours we use it.

[lorban]

Okay, I've renamed the class back to CompoundPool, explained the overflow in the javadoc instead of the name but made it a private inner class of ArrayByteBufferPool to make it more apparent that it's specific to this use-case only.

I've tried to stick to the "best is the enemy of good" mindset when working on this because:

• we have a proven perf problem with pooled buffers
• we have not (yet) a proven perf problem with pooled connections
• buffers don't need multiplexing
• QueuedPool is generic, but writing a multiplexing version would require non-trivial extra work
• Compounding ConcurrentPool with a QueuedPool overflow is very specific to the problem at hand, but solves it well

I'm not against working on a multiplexed QueuedPool nor genericizing the above, it it's IMHO outside the scope of this PR, and I'm not even convinced there's a real need for a generic overflowing pool.

[gregw]

I agree with the approach of making this PR about fixing the known problems in the ABBP. We can investigate later if we need to generalise more and come up with solutions for multiplexed pools.

I still don't like the constructor of CompoundPool, but since it is private, that is a niggle rather than a show stopper

[gregw]

LGTM