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.

Sign up for GitHub

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

Jump to bottom

Thought bubble on abstracting out multiplexing from the pool #6648

Merged

sbordet merged 0 commits into jetty-9.4.x-6603-max-concurrent-streams-exceeded from jetty-9.4.x-6603-max-concurrent-streams-exceeded-2

Aug 27, 2021

Contributor

gregw commented Aug 20, 2021

Abstract out multiplexing check from the pool, so a multiplexing pool can add a per connection check.
For Http2, this should use the existing state kept in HTTP2Session for local and remote maxSessions

gregw requested review from sbordet and lorban and removed request for sbordet

August 20, 2021 01:41

Contributor Author

gregw commented Aug 20, 2021

Hmmm I don't think the remote/local maxStreams are correctly kept upto date with SETTINGs frames???

gregw requested a review from sbordet

August 21, 2021 01:38

lorban reviewed

View reviewed changes

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

    
                              int maxUsageCount = Pool.this.maxUsageCount;

                              if (closed || !checkInUse.apply(pooled, inUseCount) || (maxUsageCount > 0 && usageCount >= maxUsageCount))

                              int maxUsageCount = Pool.this.maxUsage;

                              if (closed || !available.test(pooled, inUseCount) || (maxUsageCount > 0 && usageCount >= maxUsageCount))

Contributor

lorban Aug 23, 2021

Ouch! inUseCount is going to go through int -> Integer -> int transformations just for this call to happen... on the fast path! My guts tell me this change alone is easily going to cost ~20% throughput.

If we only take perf into account, making available a simple Predicate would be enough as we could delegate the counter to the predicate itself. But I'm not sure it can be done without breaking backward compatibility.

Contributor Author

gregw Aug 26, 2021

So we need an interface that takes int rather than use BiPredicate

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

@@ @@ -548,34 +574,17 @@ public String toString() @@
                    */
                   public class Entry
                   {
-                      // hi: positive=open/maxUsage counter; negative=closed; MIN_VALUE pending
-                      // lo: multiplexing counter
+                      // hi: negative==closed else total-usage counter; negative=closed; MIN_VALUE pending

Contributor

lorban Aug 23, 2021

negative==closed was already documented on the same line.

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

@@ @@ -270,7 +296,7 @@ public Entry reserve(int allotment) @@
                           if (space <= 0)
                               return null;
-                          if (allotment >= 0 && (getReservedCount() * getMaxMultiplex()) >= allotment)
+                          if (allotment >= 0 && getReservedCount() >= allotment)

Contributor

lorban Aug 23, 2021

I would not touch the implementation of this method at all. We already had a discussion about the allotment parameter and concluded that it was a bad idea so we shifted away from this method that we only kept for backward compatibility. IMHO there's no point in modifying the implementation of something we know is fundamentally wrong at the high-level.

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

@@ @@ -212,7 +238,7 @@ public final void setMaxMultiplex(int maxMultiplex) @@
                   @ManagedAttribute("The default maximum usage count of entries")
                   public int getMaxUsageCount()
                   {
-                      return maxUsageCount;
+                      return maxUsage;

Contributor

lorban Aug 23, 2021

It's already been concluded that max usage should not have been implemented in the low-level Pool but rather in the higher level ConnectionPool, like what we did for the max-time-to-live. If you're considering deprecating the max multiplex, this other property should be deprecated too.

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

+                      this.available = available == null ? this::isAvailable : available;
+                  }
+                  private boolean isAvailable(T item, Integer inUse)

Contributor

lorban Aug 23, 2021

I'd rather inline this method as it only is used as a lambda.

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

    
                      // hi: positive=open/maxUsage counter; negative=closed; MIN_VALUE pending

                      // lo: multiplexing counter

                      // hi: negative==closed else total-usage counter; negative=closed; MIN_VALUE pending

                      // lo: in-use counter

                      private final AtomicBiInteger state;

Contributor

lorban Aug 23, 2021

If we got rid of the two counters this AtomicBiInteger keeps track of, we could simplify it down to a tri-state PENDING / OPEN / CLOSED that would measurably boost performance of highly contended pools.

Contributor Author

gregw commented Aug 27, 2021

@sbordet @lorban I've checked in a big refactor (work in progress) of this throught bubble along the lines of a discussion I had with simone this morning. Please read these notes before looking at the code.

The main change to Pool is that the Entry is now extensible. The idea being that for non-connection pool stuff we have a really simple single use Entry, but that connection pool can have it's own more complex entry that supports multiplexing and max usage.
However FOR NOW, I have left multiplexing ability in the util Pool, this is because of all the existing methods and because ConnectionPool has Pool in it's API. So as a first step, the Pool has two Entry impls: MultiUseEntry and SingleUseEntry and if the setMaxXxxx methods have not been used, then it uses the SingleUseEntry
I've gone through the connection pools and where possible stopped them creating their own pool instance and instead mostly pass around the pool strategy they want to use.
The HTTP2 connection types extends the new Pool.getMaxMultiplex(T item) method to look up the max in the HTTP2Session, so per connection state is not duplicated.

I've run out of time, so I have no idea how close this is to working.

sbordet merged commit 7733d0f into jetty-9.4.x-6603-max-concurrent-streams-exceeded

sbordet mentioned this pull request

Fixes #6603 - HTTP/2 max local stream count exceeded #6639

Merged

joakime deleted the jetty-9.4.x-6603-max-concurrent-streams-exceeded-2 branch

September 15, 2021 17:40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment