Improve client http connection pools #4975

lorban · 2020-06-17T12:45:12Z

Fixes #4809 : introduced setMaxUsageCount(int) on DuplexConnectionPool and MultiplexConnectionPool
Fixes #3248 : introduced preCreateConnections(int) on AbstractConnectionPool

Modify the HTTP client's connection pooling internal mechanism to stop using queues and removing pooled connections from the pool but rather mark them as they're used and released. This improves performance by 2.5X (worst case) up to 100X (best case).

The pooling algorithm should be generic enough to be reusable for any kind of object pooling (ByteBuffers...)

jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java

jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java

.../test-http-client-transport/src/test/java/org/eclipse/jetty/client/ConnectionPoolHelper.java

jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java

gregw · 2020-07-15T08:33:01Z

jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java

-            int pending = AtomicBiInteger.getHi(encoded);
-            int total = AtomicBiInteger.getLo(encoded);
+            int pending = pendingConnectionCount.get();
+            int total = pool.size() + pending;


This still needs to be resolved.

lorban · 2020-07-15T09:50:54Z

@gregw regarding the int total = pool.size() + pending; race condition, I've already modified the code to handle it this way:

The callback now adds the connection to the pool before decrementing the pending connection count. This prevents the total variable to ever overshoot the configured maxConnections with the only drawback that a race condition at the wrong time may lead the pool to not create an extra connection while there is still room in the pool. The next time the pool needs an extra connection it will attempt to create one and will eventually succeed.

Since the above situation is transient, and only happens during warmup or a spike of requests, it wouldn't take long for the pool to eventually reach its max size: I do not think that more than a few attempts would end up not creating the connections and the moer you pile up work, the shorter this situation will last.

IMHO it's not worth duplicating the pool size in AbstractConnectionPool and maintaining it strictly in sync with pendingConnectionCount.

This is yet another case of "embracing the race" rather than handling it.

gregw · 2020-07-15T15:50:48Z

@lorban Still don't like it!
How about the following - we get rid of the whole concept of pending. Instead we immediately add a new entry, but one that has already been acquired so that nothing will take it out of the pool. Once the connection is actually completed we release it, otherwise if the connection fails we take it out. Then we don't have two size+pending, we just have size.

lorban · 2020-07-16T09:26:52Z

@gregw I took your idea and ran with it.

The Pool now has a reserve-then-enable model: you first reserve a slot which creates a closed entry that cannot be acquired, then you can either enable the entry which makes it acquirable, or remove it from the pool.

gregw

Much better, but I'd do the total>max test within the addReservation lock.

jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java

jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java

gregw

getting closer

jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java

gregw

I'm down to just some naming & documentation niggles, plus a thought bubble for another optimisation.

@sbordet has asked some questions that I'd like to see the answers to.. maybe we should have a final hangout on this?, but overall I'm +1

jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java

jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java

jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java

jetty-client/src/main/java/org/eclipse/jetty/client/ConnectionPool.java

jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java

sbordet · 2020-07-28T09:02:05Z

jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java


-    protected Connection active(Connection connection)
+    protected Connection activate(int offset)


I still have concerns about this - I would like to move the "index" to round-robin only, as others don't need it.

jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java

sbordet · 2020-07-28T10:47:58Z

jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java

+        {
+            reservationLock.unlock();
+        }
+    }


Isn't this method very costly now? with the stream filtering and the lock? Can't we use the entry state to "reserve"?

For a certain definition of costly, yes: this method takes a lock and iterates the shared list. But over the Grand Scheme of Things, adding entries to the pool should be a rather uncommon event, I don't think adding new pooled entries every millisecond is a sane behavior after warmup is done. So overall, I don't think it really matters.

As discussed, I think the lock can be made smaller (just rename it from reservationLock to lock as it's also used to protect closed).

Only 138-142 needs to be guarded by the lock.

sbordet · 2020-07-28T12:12:41Z

jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java

+    @Override
+    public void close()
+    {
+        reservationLock.lock();


Why the lock here? I mean that it seems to protect close but not everywhere, and works on the same variables that are used in other methods but not protected by this lock.

The only thing that must not happen concurrently is the addition of entries to the shared list, and additions while it is being cleaned up as this could cause:

overshooting maxEntries

overshooting maxReservations

creating entries in a closed pool

while concurrently acquiring / releasing / removing entries can be safely handled. So this is what this lock is about.

As discussed, the lock can be reduced to flip closed, copy the entries, and clear sharedList.
The iteration can be outside.

sbordet

Almost there.

jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java

sbordet · 2020-07-29T19:19:27Z

jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java

+    @Override
+    public void close()
+    {
+        reservationLock.lock();


As discussed, the lock can be reduced to flip closed, copy the entries, and clear sharedList.
The iteration can be outside.

jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java

sbordet

LGTM.

Please remember to use AutoLock when you merge to 10 in class Pool which is a new class (and you will have to change the copyright header as well).

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw · 2020-08-03T11:47:59Z

\o/ So are improved ByteBufferPools next?

Implemented as part of #4975. Added a test case that proves that the connection is closed when the max usage count is reached. Improved logging. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

lorban changed the title ~~Improve http connection pools~~ Improve client http connection pools Jun 22, 2020

lorban marked this pull request as ready for review June 30, 2020 14:24

sbordet self-requested a review July 3, 2020 14:41

sbordet requested changes Jul 7, 2020

View reviewed changes

sbordet reviewed Jul 7, 2020

View reviewed changes

jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java Outdated Show resolved Hide resolved

lorban requested review from gregw and sbordet July 10, 2020 08:11

gregw requested changes Jul 15, 2020

View reviewed changes

lorban requested a review from gregw July 15, 2020 09:51

gregw requested changes Jul 16, 2020

View reviewed changes

jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java Outdated Show resolved Hide resolved

jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java Outdated Show resolved Hide resolved

lorban requested a review from gregw July 16, 2020 15:46

gregw requested changes Jul 22, 2020

View reviewed changes

jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java Outdated Show resolved Hide resolved

lorban requested a review from gregw July 27, 2020 09:23

gregw approved these changes Jul 27, 2020

View reviewed changes

sbordet requested changes Jul 28, 2020

View reviewed changes

sbordet requested changes Jul 29, 2020

View reviewed changes

sbordet approved these changes Jul 31, 2020

View reviewed changes

Re-implement HTTP connection pooling with a non-queuing algorithm

e9dad97

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

sbordet merged commit 344cdf5 into jetty:jetty-9.4.x Jul 31, 2020

sbordet added a commit that referenced this pull request Jul 31, 2020

Fixes after review of #4975.

d2df0fc

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet linked an issue Aug 12, 2020 that may be closed by this pull request

Set a max number of requests per connection #4809

Closed

sbordet mentioned this pull request Aug 13, 2020

Introduce pre-opening of connections in MultiplexedConnectionPool #3248

Closed

lorban deleted the http-connection-pools branch August 17, 2020 07:48

sbordet mentioned this pull request Sep 1, 2020

Review RoundRobinConnectionPool #5217

Closed

joakime mentioned this pull request Oct 9, 2020

Custom Connection Pool not working after jetty upgrade #5416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve client http connection pools #4975

Improve client http connection pools #4975

lorban commented Jun 17, 2020 •

edited

gregw Jul 15, 2020

lorban commented Jul 15, 2020

gregw commented Jul 15, 2020

lorban commented Jul 16, 2020 •

edited

gregw left a comment

gregw left a comment

gregw left a comment

sbordet Jul 28, 2020

sbordet Jul 28, 2020

lorban Jul 29, 2020 •

edited

sbordet Jul 29, 2020

sbordet Jul 28, 2020

lorban Jul 29, 2020 •

edited

sbordet Jul 29, 2020

sbordet left a comment

sbordet Jul 29, 2020

sbordet left a comment

gregw commented Aug 3, 2020


		protected Connection active(Connection connection)
		protected Connection activate(int offset)

Improve client http connection pools #4975

Improve client http connection pools #4975

Conversation

lorban commented Jun 17, 2020 • edited

gregw Jul 15, 2020

Choose a reason for hiding this comment

lorban commented Jul 15, 2020

gregw commented Jul 15, 2020

lorban commented Jul 16, 2020 • edited

gregw left a comment

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment

sbordet Jul 28, 2020

Choose a reason for hiding this comment

sbordet Jul 28, 2020

Choose a reason for hiding this comment

lorban Jul 29, 2020 • edited

Choose a reason for hiding this comment

sbordet Jul 29, 2020

Choose a reason for hiding this comment

sbordet Jul 28, 2020

Choose a reason for hiding this comment

lorban Jul 29, 2020 • edited

Choose a reason for hiding this comment

sbordet Jul 29, 2020

Choose a reason for hiding this comment

sbordet left a comment

Choose a reason for hiding this comment

sbordet Jul 29, 2020

Choose a reason for hiding this comment

sbordet left a comment

Choose a reason for hiding this comment

gregw commented Aug 3, 2020

lorban commented Jun 17, 2020 •

edited

lorban commented Jul 16, 2020 •

edited

lorban Jul 29, 2020 •

edited

lorban Jul 29, 2020 •

edited