Hi Mark, On Fri, Sep 25, 2020 at 4:35 PM <ma...@apache.org> wrote:
> This is an automated email from the ASF dual-hosted git repository. > > markt pushed a commit to branch master > in repository https://gitbox.apache.org/repos/asf/tomcat.git > > commit 954cbffa73efe6e546a07c84ade97a3b9b38a893 > Author: Mark Thomas <ma...@apache.org> > AuthorDate: Fri Sep 25 14:14:06 2020 +0100 > > Refactor the pruning so more stream info is retained for longer > > The memory impact of this is mitigated by the previous changes to > replace closed Stream instances with RecycledStream instances. > --- > .../apache/coyote/http2/Http2UpgradeHandler.java | 89 > ++++++++++++---------- > 1 file changed, 49 insertions(+), 40 deletions(-) > > diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java > b/java/org/apache/coyote/http2/Http2UpgradeHandler.java > index a023fe0..49115d3 100644 > --- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java > +++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java > @@ -30,6 +30,7 @@ import java.util.TreeSet; > import java.util.concurrent.ConcurrentHashMap; > import java.util.concurrent.ConcurrentLinkedQueue; > import java.util.concurrent.ConcurrentMap; > +import java.util.concurrent.ConcurrentSkipListMap; > import java.util.concurrent.atomic.AtomicInteger; > import java.util.concurrent.atomic.AtomicLong; > import java.util.concurrent.atomic.AtomicReference; > @@ -123,7 +124,7 @@ class Http2UpgradeHandler extends AbstractStream > implements InternalHttpUpgradeH > private HpackDecoder hpackDecoder; > private HpackEncoder hpackEncoder; > > - private final ConcurrentMap<Integer,AbstractNonZeroStream> streams = > new ConcurrentHashMap<>(); > + private final ConcurrentMap<Integer,AbstractNonZeroStream> streams = > new ConcurrentSkipListMap<>(); > In my branch I also moved to ConcurrentSkipListMap: https://github.com/martin-g/tomcat/commit/08cca44fbb9bd05e35c5655ff7d6ea9781bc28fa I did it because #closeIdleStreams() was appearing in the profiler. With this change it is no more in the profiler results! Now the only drawback is that streams.size() (called in org.apache.coyote.http2.Http2UpgradeHandler#pruneClosedStreams()) is no longer a constant-time operation. > protected final AtomicInteger activeRemoteStreamCount = new > AtomicInteger(0); > // Start at -1 so the 'add 2' logic in closeIdleStreams() works > private volatile int maxActiveRemoteStreamId = -1; > @@ -1207,78 +1208,86 @@ class Http2UpgradeHandler extends AbstractStream > implements InternalHttpUpgradeH > // 2. Completed streams used for a request with children > // 3. Closed final streams > // > - // Steps 1 and 2 will always be completed. > - // Step 3 will be completed to the minimum extent necessary to > bring the > - // total number of streams under the limit. > + // The pruning halts as soon as enough streams have been pruned. > > // Use these sets to track the different classes of streams > - TreeSet<Integer> candidatesStepOne = new TreeSet<>(); > TreeSet<Integer> candidatesStepTwo = new TreeSet<>(); > TreeSet<Integer> candidatesStepThree = new TreeSet<>(); > > - for (Entry<Integer, AbstractNonZeroStream> entry : > streams.entrySet()) { > - AbstractNonZeroStream stream = entry.getValue(); > + // Step 1 > + // Iterator is in key order so we automatically have the oldest > streams > + // first > + for (AbstractNonZeroStream stream : streams.values()) { > // Never remove active streams > if (stream instanceof Stream && ((Stream) stream).isActive()) > { > continue; > } > > - final Integer streamIdentifier = entry.getKey(); > if (stream.isClosedFinal()) { > // This stream went from IDLE to CLOSED and is likely to > have > // been created by the client as part of the priority > tree. > - candidatesStepThree.add(streamIdentifier); > + // Candidate for steo 3. > + candidatesStepThree.add(stream.getIdentifier()); > } else if (stream.getChildStreams().size() == 0) { > - // Closed, no children > - candidatesStepOne.add(streamIdentifier); > - } else { > - // Closed, with children > - candidatesStepTwo.add(streamIdentifier); > - } > - } > - > - // Process the step one list > - for (Integer streamIdToRemove : candidatesStepOne) { > - // Remove this childless stream > - AbstractNonZeroStream removedStream = > streams.remove(streamIdToRemove); > - removedStream.detachFromParent(); > - toClose--; > - if (log.isDebugEnabled()) { > - log.debug(sm.getString("upgradeHandler.pruned", > connectionId, streamIdToRemove)); > - } > - > - // Did this make the parent childless? > - AbstractStream parent = removedStream.getParentStream(); > - while (parent instanceof Stream && !((Stream) > parent).isActive() && > - !((Stream) parent).isClosedFinal() && > parent.getChildStreams().size() == 0) { > - streams.remove(parent.getIdentifier()); > - parent.detachFromParent(); > - toClose--; > + // Prune it > + AbstractStream parent = stream.getParentStream(); > + streams.remove(stream.getIdentifier()); > + stream.detachFromParent(); > if (log.isDebugEnabled()) { > - log.debug(sm.getString("upgradeHandler.pruned", > connectionId, streamIdToRemove)); > + log.debug(sm.getString("upgradeHandler.pruned", > connectionId, stream.getIdAsString())); > + } > + if (--toClose < 1) { > + return; > } > - // Also need to remove this stream from the p2 list > - candidatesStepTwo.remove(parent.getIdentifier()); > - parent = parent.getParentStream(); > + > + // If removing this child made the parent childless then > see if > + // the parent can be removed. > + // Don't try and remove Stream 0 as that is the connection > + // Don't try and remove 'newer' streams. We'll get to > them as we > + // work through the ordered list of streams. > + while (toClose > 0 && parent.getIdAsInt() > 0 && > parent.getIdAsInt() < stream.getIdAsInt() && > + parent.getChildStreams().isEmpty()) { > + // This case is safe since we know parent ID > 0 > therefore > + // this isn't the connection > + stream = (AbstractNonZeroStream) parent; > + parent = stream.getParentStream(); > + streams.remove(stream.getIdentifier()); > + stream.detachFromParent(); > + if (log.isDebugEnabled()) { > + log.debug(sm.getString("upgradeHandler.pruned", > connectionId, stream.getIdAsString())); > + } > + if (--toClose < 1) { > + return; > + } > + // Also need to remove this stream from the step 2 > list > + candidatesStepTwo.remove(stream.getIdentifier()); > + } > + } else { > + // Closed, with children. Candidate for step 2. > + candidatesStepTwo.add(stream.getIdentifier()); > } > } > > // Process the P2 list > for (Integer streamIdToRemove : candidatesStepTwo) { > removeStreamFromPriorityTree(streamIdToRemove); > - toClose--; > if (log.isDebugEnabled()) { > log.debug(sm.getString("upgradeHandler.pruned", > connectionId, streamIdToRemove)); > } > + if (--toClose < 1) { > + return; > + } > } > > while (toClose > 0 && candidatesStepThree.size() > 0) { > Integer streamIdToRemove = candidatesStepThree.pollLast(); > removeStreamFromPriorityTree(streamIdToRemove); > - toClose--; > if (log.isDebugEnabled()) { > log.debug(sm.getString("upgradeHandler.prunedPriority", > connectionId, streamIdToRemove)); > } > + if (--toClose < 1) { > + return; > + } > } > > if (toClose > 0) { > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > >
