This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 8a0ab84ea67025ba75e545e5456fc28f051ace70
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Fri Sep 25 12:26:40 2020 +0100
Fully replace Stream with RecycledStream
Profiler (YourKit) suggests retain memory for Stream ~60k whereas
RecycledStream ins ~300 bytes.
---
.../apache/coyote/http2/AbstractNonZeroStream.java | 37 +++++++++++--
.../apache/coyote/http2/Http2UpgradeHandler.java | 64 +++++++++++++---------
java/org/apache/coyote/http2/RecycledStream.java | 4 +-
java/org/apache/coyote/http2/Stream.java | 2 +-
4 files changed, 74 insertions(+), 33 deletions(-)
diff --git a/java/org/apache/coyote/http2/AbstractNonZeroStream.java
b/java/org/apache/coyote/http2/AbstractNonZeroStream.java
index bad3b76..d85840d 100644
--- a/java/org/apache/coyote/http2/AbstractNonZeroStream.java
+++ b/java/org/apache/coyote/http2/AbstractNonZeroStream.java
@@ -33,19 +33,17 @@ abstract class AbstractNonZeroStream extends AbstractStream
{
protected final StreamStateMachine state;
- private volatile int weight;
+ private volatile int weight = Constants.DEFAULT_WEIGHT;
AbstractNonZeroStream(String connectionId, Integer identifier) {
super(identifier);
- this.weight = Constants.DEFAULT_WEIGHT;
this.state = new StreamStateMachine(connectionId, getIdAsString());
}
- AbstractNonZeroStream(Integer identifier, int weight, StreamStateMachine
state) {
+ AbstractNonZeroStream(Integer identifier, StreamStateMachine state) {
super(identifier);
- this.weight = weight;
this.state = state;
}
@@ -56,6 +54,13 @@ abstract class AbstractNonZeroStream extends AbstractStream {
}
+ /*
+ * General method used when reprioritising a stream and care needs to be
+ * taken not to create circular references.
+ *
+ * Changes to the priority tree need to be sychronized at the connection
+ * level. This is the caller's responsibility.
+ */
final void rePrioritise(AbstractStream parent, boolean exclusive, int
weight) {
if (log.isDebugEnabled()) {
log.debug(sm.getString("stream.reprioritisation.debug",
@@ -90,6 +95,9 @@ abstract class AbstractNonZeroStream extends AbstractStream {
/*
* Used when removing closed streams from the tree and we know there is no
* need to check for circular references.
+ *
+ * Changes to the priority tree need to be sychronized at the connection
+ * level. This is the caller's responsibility.
*/
final void rePrioritise(AbstractStream parent, int weight) {
if (log.isDebugEnabled()) {
@@ -103,6 +111,27 @@ abstract class AbstractNonZeroStream extends
AbstractStream {
}
+ /*
+ * Used when "recycling" a stream and replacing a Stream instance with a
+ * RecycledStream instance.
+ *
+ * Replace this stream with the provided stream in the parent/child
+ * hierarchy.
+ *
+ * Changes to the priority tree need to be sychronized at the connection
+ * level. This is the caller's responsibility.
+ */
+ void replaceStream(AbstractNonZeroStream replacement) {
+ replacement.setParentStream(getParentStream());
+ detachFromParent();
+ for (AbstractNonZeroStream child : getChildStreams()) {
+ replacement.addChild(child);
+ }
+ getChildStreams().clear();
+ replacement.weight = weight;
+ }
+
+
final boolean isClosedFinal() {
return state.isClosedFinal();
}
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 751637c..295e9aa 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -93,6 +93,8 @@ public class Http2UpgradeHandler extends AbstractStream
implements InternalHttpU
private static final HeaderSink HEADER_SINK = new HeaderSink();
+ private final Object priorityTreeLock = new Object();
+
private final String connectionId;
private final Http2Protocol protocol;
@@ -103,8 +105,7 @@ public class Http2UpgradeHandler extends AbstractStream
implements InternalHttpU
private volatile Http2Parser parser;
// Simple state machine (sequence of states)
- private AtomicReference<ConnectionState> connectionState =
- new AtomicReference<>(ConnectionState.NEW);
+ private AtomicReference<ConnectionState> connectionState = new
AtomicReference<>(ConnectionState.NEW);
private volatile long pausedNanoTime = Long.MAX_VALUE;
/**
@@ -1166,7 +1167,7 @@ public class Http2UpgradeHandler extends AbstractStream
implements InternalHttpU
newStreamsSinceLastPrune = 0;
// RFC 7540, 5.3.4 endpoints should maintain state for at least the
- // maximum number of concurrent streams
+ // maximum number of concurrent streams.
long max = localSettings.getMaxConcurrentStreams();
if (log.isDebugEnabled()) {
@@ -1174,9 +1175,13 @@ public class Http2UpgradeHandler extends AbstractStream
implements InternalHttpU
Long.toString(max), Integer.toString(streams.size())));
}
- // Allow an additional 10% for closed streams that are used in the
- // priority tree
- max = max + max / 10;
+ // Only need ~+10% for streams that are in the priority tree,
+ // Ideally need to retain information for a "significant" amount of
time
+ // after sending END_STREAM (RFC 7540, page 20) so we detect potential
+ // connection error. 5x seems reasonable. The client will have had
+ // plenty of opportunity to process the END_STREAM if another 5x max
+ // concurrent streams have been processed.
+ max = max * 5;
if (max > Integer.MAX_VALUE) {
max = Integer.MAX_VALUE;
}
@@ -1273,27 +1278,29 @@ public class Http2UpgradeHandler extends AbstractStream
implements InternalHttpU
private void removeStreamFromPriorityTree(Integer streamIdToRemove) {
- AbstractNonZeroStream streamToRemove =
streams.remove(streamIdToRemove);
- // Move the removed Stream's children to the removed Stream's
- // parent.
- Set<AbstractNonZeroStream> children = streamToRemove.getChildStreams();
- if (children.size() == 1) {
- // Shortcut
- streamToRemove.getChildStreams().iterator().next().rePrioritise(
- streamToRemove.getParentStream(),
streamToRemove.getWeight());
- } else {
- int totalWeight = 0;
- for (AbstractNonZeroStream child : children) {
- totalWeight += child.getWeight();
- }
- for (AbstractNonZeroStream child : children) {
+ synchronized (priorityTreeLock) {
+ AbstractNonZeroStream streamToRemove =
streams.remove(streamIdToRemove);
+ // Move the removed Stream's children to the removed Stream's
+ // parent.
+ Set<AbstractNonZeroStream> children =
streamToRemove.getChildStreams();
+ if (children.size() == 1) {
+ // Shortcut
children.iterator().next().rePrioritise(
- streamToRemove.getParentStream(),
- streamToRemove.getWeight() * child.getWeight() /
totalWeight);
+ streamToRemove.getParentStream(),
streamToRemove.getWeight());
+ } else {
+ int totalWeight = 0;
+ for (AbstractNonZeroStream child : children) {
+ totalWeight += child.getWeight();
+ }
+ for (AbstractNonZeroStream child : children) {
+ children.iterator().next().rePrioritise(
+ streamToRemove.getParentStream(),
+ streamToRemove.getWeight() * child.getWeight() /
totalWeight);
+ }
}
+ streamToRemove.detachFromParent();
+ children.clear();
}
- streamToRemove.detachFromParent();
- streamToRemove.getChildStreams().clear();
}
@@ -1649,7 +1656,9 @@ public class Http2UpgradeHandler extends AbstractStream
implements InternalHttpU
if (parentStream == null) {
parentStream = this;
}
- abstractNonZeroStream.rePrioritise(parentStream, exclusive, weight);
+ synchronized (priorityTreeLock) {
+ abstractNonZeroStream.rePrioritise(parentStream, exclusive,
weight);
+ }
}
@@ -1845,7 +1854,10 @@ public class Http2UpgradeHandler extends AbstractStream
implements InternalHttpU
void replaceStream(AbstractNonZeroStream original, AbstractNonZeroStream
replacement) {
- streams.replace(original.getIdentifier(), replacement);
+ synchronized (priorityTreeLock) {
+ streams.replace(original.getIdentifier(), replacement);
+ original.replaceStream(replacement);
+ }
}
diff --git a/java/org/apache/coyote/http2/RecycledStream.java
b/java/org/apache/coyote/http2/RecycledStream.java
index e6f90f0..8f9cc06 100644
--- a/java/org/apache/coyote/http2/RecycledStream.java
+++ b/java/org/apache/coyote/http2/RecycledStream.java
@@ -24,8 +24,8 @@ class RecycledStream extends AbstractNonZeroStream {
private final String connectionId;
- RecycledStream(String connectionId, Integer identifier, int weight,
StreamStateMachine state) {
- super(identifier, weight, state);
+ RecycledStream(String connectionId, Integer identifier, StreamStateMachine
state) {
+ super(identifier, state);
this.connectionId = connectionId;
}
diff --git a/java/org/apache/coyote/http2/Stream.java
b/java/org/apache/coyote/http2/Stream.java
index 22d62ed..28ab46f 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -655,7 +655,7 @@ public class Stream extends AbstractNonZeroStream
implements HeaderEmitter {
if (log.isDebugEnabled()) {
log.debug(sm.getString("stream.recycle", getConnectionId(),
getIdAsString()));
}
- handler.replaceStream(this, new RecycledStream(getConnectionId(),
getIdentifier(), getWeight(), state));
+ handler.replaceStream(this, new RecycledStream(getConnectionId(),
getIdentifier(), state));
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
