You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2020/09/30 12:46:28 UTC
[tomcat] branch 9.0.x updated (8dcf599 -> f258efe)
This is an automated email from the ASF dual-hosted git repository.
markt pushed a change to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git.
from 8dcf599 HTTP2 log statement fixes:
new 0d79ae5 Reduce memory footprint of closed http/2 streams
new e1a53fb Reduce memory footprint of closed http/2 streams
new 3783dc8 Reduce memory footprint of closed http/2 streams
new 496b3cf Reduce memory footprint of closed http/2 streams
new a6e8428 Reduce memory footprint of closed http/2 streams
new 2b2ac4f Reduce memory footprint of closed http/2 streams
new d1f204e Fully replace Stream with RecycledStream
new 83f5e03 Refactor the pruning so more stream info is retained for longer
new a4054eb Update changelog
new f258efe Optimize the iteration when closing idle streams
The 10 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../apache/coyote/http2/AbstractNonZeroStream.java | 143 +++++++++++
java/org/apache/coyote/http2/AbstractStream.java | 9 +-
.../apache/coyote/http2/Http2UpgradeHandler.java | 264 +++++++++++++--------
...tionSettingsRemote.java => RecycledStream.java} | 23 +-
java/org/apache/coyote/http2/Stream.java | 90 +------
.../apache/coyote/http2/StreamStateMachine.java | 21 +-
webapps/docs/changelog.xml | 5 +
7 files changed, 348 insertions(+), 207 deletions(-)
create mode 100644 java/org/apache/coyote/http2/AbstractNonZeroStream.java
copy java/org/apache/coyote/http2/{ConnectionSettingsRemote.java => RecycledStream.java} (61%)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 03/10: Reduce memory footprint of closed http/2 streams
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 3783dc83065414cd85179e9a9014da44780e14b6
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Thu Sep 24 17:50:43 2020 +0100
Reduce memory footprint of closed http/2 streams
This refactoring replaces closed streams with a new RecycledStream
object and changes the mechanism used to look up known streams.
Pull-up isClosedFinal()
---
java/org/apache/coyote/http2/AbstractNonZeroStream.java | 1 +
java/org/apache/coyote/http2/RecycledStream.java | 8 ++++++++
java/org/apache/coyote/http2/Stream.java | 1 +
3 files changed, 10 insertions(+)
diff --git a/java/org/apache/coyote/http2/AbstractNonZeroStream.java b/java/org/apache/coyote/http2/AbstractNonZeroStream.java
index ce08cc5..70cfe35 100644
--- a/java/org/apache/coyote/http2/AbstractNonZeroStream.java
+++ b/java/org/apache/coyote/http2/AbstractNonZeroStream.java
@@ -97,4 +97,5 @@ abstract class AbstractNonZeroStream extends AbstractStream {
this.weight = weight;
}
+ abstract boolean isClosedFinal();
}
diff --git a/java/org/apache/coyote/http2/RecycledStream.java b/java/org/apache/coyote/http2/RecycledStream.java
index 1e630df..dbbdc10 100644
--- a/java/org/apache/coyote/http2/RecycledStream.java
+++ b/java/org/apache/coyote/http2/RecycledStream.java
@@ -23,10 +23,12 @@ package org.apache.coyote.http2;
class RecycledStream extends AbstractNonZeroStream {
private final String connectionId;
+ private final boolean closedFinal;
RecycledStream(Stream stream) {
super(stream.getIdentifier(), stream.getWeight());
connectionId = stream.getConnectionId();
+ closedFinal = stream.isClosedFinal();
}
@@ -34,4 +36,10 @@ class RecycledStream extends AbstractNonZeroStream {
String getConnectionId() {
return connectionId;
}
+
+
+ @Override
+ boolean isClosedFinal() {
+ return closedFinal;
+ }
}
diff --git a/java/org/apache/coyote/http2/Stream.java b/java/org/apache/coyote/http2/Stream.java
index ef7ed29..489ce53 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -646,6 +646,7 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter {
}
+ @Override
final boolean isClosedFinal() {
return state.isClosedFinal();
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 05/10: Reduce memory footprint of closed http/2 streams
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit a6e84280ac145ab14f86a2cbe37238f9acda31d6
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Thu Sep 24 21:36:19 2020 +0100
Reduce memory footprint of closed http/2 streams
This refactoring replaces closed streams with a new RecycledStream
object and changes the mechanism used to look up known streams.
Refactor StreamStateMachine to remove reference to Stream
---
java/org/apache/coyote/http2/Stream.java | 2 +-
.../org/apache/coyote/http2/StreamStateMachine.java | 21 ++++++++++++---------
2 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/java/org/apache/coyote/http2/Stream.java b/java/org/apache/coyote/http2/Stream.java
index 8aa4a7b..f6210d7 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -97,7 +97,7 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter {
this.handler = handler;
handler.addChild(this);
setWindowSize(handler.getRemoteSettings().getInitialWindowSize());
- state = new StreamStateMachine(this);
+ state = new StreamStateMachine(getConnectionId(), getIdAsString());
if (coyoteRequest == null) {
// HTTP/2 new request
this.coyoteRequest = new Request();
diff --git a/java/org/apache/coyote/http2/StreamStateMachine.java b/java/org/apache/coyote/http2/StreamStateMachine.java
index 851e1a8..72028fd 100644
--- a/java/org/apache/coyote/http2/StreamStateMachine.java
+++ b/java/org/apache/coyote/http2/StreamStateMachine.java
@@ -39,12 +39,15 @@ class StreamStateMachine {
private static final Log log = LogFactory.getLog(StreamStateMachine.class);
private static final StringManager sm = StringManager.getManager(StreamStateMachine.class);
- private final Stream stream;
+ private final String connectionId;
+ private final String streamId;
+
private State state;
- StreamStateMachine(Stream stream) {
- this.stream = stream;
+ StreamStateMachine(String connectionId, String streamId) {
+ this.connectionId = connectionId;
+ this.streamId = streamId;
stateChange(null, State.IDLE);
}
@@ -92,7 +95,7 @@ class StreamStateMachine {
public synchronized void sendReset() {
if (state == State.IDLE) {
throw new IllegalStateException(sm.getString("streamStateMachine.debug.change",
- stream.getConnectionId(), stream.getIdAsString(), state));
+ connectionId, streamId, state));
}
if (state.canReset()) {
stateChange(state, State.CLOSED_RST_TX);
@@ -109,8 +112,8 @@ class StreamStateMachine {
if (state == oldState) {
state = newState;
if (log.isDebugEnabled()) {
- log.debug(sm.getString("streamStateMachine.debug.change", stream.getConnectionId(),
- stream.getIdAsString(), oldState, newState));
+ log.debug(sm.getString("streamStateMachine.debug.change", connectionId,
+ streamId, oldState, newState));
}
}
}
@@ -122,12 +125,12 @@ class StreamStateMachine {
if (!isFrameTypePermitted(frameType)) {
if (state.connectionErrorForInvalidFrame) {
throw new ConnectionException(sm.getString("streamStateMachine.invalidFrame",
- stream.getConnectionId(), stream.getIdAsString(), state, frameType),
+ connectionId, streamId, state, frameType),
state.errorCodeForInvalidFrame);
} else {
throw new StreamException(sm.getString("streamStateMachine.invalidFrame",
- stream.getConnectionId(), stream.getIdAsString(), state, frameType),
- state.errorCodeForInvalidFrame, stream.getIdAsInt());
+ connectionId, streamId, state, frameType),
+ state.errorCodeForInvalidFrame, Integer.parseInt(streamId));
}
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 10/10: Optimize the iteration when closing idle streams
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit f258efef6c1e9c71ceaf1f50e1dc354f2514ab3a
Author: Martin Tzvetanov Grigorov <mg...@apache.org>
AuthorDate: Mon Sep 28 14:53:25 2020 +0300
Optimize the iteration when closing idle streams
---
java/org/apache/coyote/http2/Http2UpgradeHandler.java | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 9f71af9..a1002c1 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -29,7 +29,7 @@ import java.util.Set;
import java.util.TreeSet;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentLinkedQueue;
-import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ConcurrentNavigableMap;
import java.util.concurrent.ConcurrentSkipListMap;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
@@ -124,7 +124,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
private HpackDecoder hpackDecoder;
private HpackEncoder hpackEncoder;
- private final ConcurrentMap<Integer,AbstractNonZeroStream> streams = new ConcurrentSkipListMap<>();
+ private final ConcurrentNavigableMap<Integer,AbstractNonZeroStream> streams = new ConcurrentSkipListMap<>();
protected final AtomicInteger activeRemoteStreamCount = new AtomicInteger(0);
// Start at -1 so the 'add 2' logic in closeIdleStreams() works
private volatile int maxActiveRemoteStreamId = -1;
@@ -1530,12 +1530,12 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
private void closeIdleStreams(int newMaxActiveRemoteStreamId) {
- for (Entry<Integer,AbstractNonZeroStream> entry : streams.entrySet()) {
- int id = entry.getKey().intValue();
- if (id > maxActiveRemoteStreamId && id < newMaxActiveRemoteStreamId) {
- if (entry.getValue() instanceof Stream) {
- ((Stream) entry.getValue()).closeIfIdle();
- }
+ final ConcurrentNavigableMap<Integer, AbstractNonZeroStream> subMap = streams.subMap(
+ Integer.valueOf(maxActiveRemoteStreamId), false,
+ Integer.valueOf(newMaxActiveRemoteStreamId), false);
+ for (AbstractNonZeroStream stream : subMap.values()) {
+ if (stream instanceof Stream) {
+ ((Stream)stream).closeIfIdle();
}
}
maxActiveRemoteStreamId = newMaxActiveRemoteStreamId;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 01/10: Reduce memory footprint of closed http/2 streams
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 0d79ae569f3fd2646ff76d395f0f56e98b798a98
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Wed Sep 23 19:59:13 2020 +0100
Reduce memory footprint of closed http/2 streams
This refactoring replaces closed streams with a new RecycledStream
object and changes the mechanism used to look up known streams.
Initial changes to object hierarchy in preparation for further
changes.
---
.../apache/coyote/http2/AbstractNonZeroStream.java | 29 ++++++++++++++
java/org/apache/coyote/http2/AbstractStream.java | 3 +-
java/org/apache/coyote/http2/RecycledStream.java | 45 ++++++++++++++++++++++
java/org/apache/coyote/http2/Stream.java | 2 +-
4 files changed, 77 insertions(+), 2 deletions(-)
diff --git a/java/org/apache/coyote/http2/AbstractNonZeroStream.java b/java/org/apache/coyote/http2/AbstractNonZeroStream.java
new file mode 100644
index 0000000..63fb5e7
--- /dev/null
+++ b/java/org/apache/coyote/http2/AbstractNonZeroStream.java
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.coyote.http2;
+
+
+/**
+ * Base class for all streams other than stream 0, the connection. Primarily
+ * provides functionality shared between full Stream and RecycledStream.
+ */
+abstract class AbstractNonZeroStream extends AbstractStream {
+
+ AbstractNonZeroStream(Integer identifier) {
+ super(identifier);
+ }
+}
diff --git a/java/org/apache/coyote/http2/AbstractStream.java b/java/org/apache/coyote/http2/AbstractStream.java
index 088d5b0..21963ee 100644
--- a/java/org/apache/coyote/http2/AbstractStream.java
+++ b/java/org/apache/coyote/http2/AbstractStream.java
@@ -25,7 +25,8 @@ import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.util.res.StringManager;
/**
- * Used to managed prioritisation.
+ * Base class for all streams including the connection (referred to as Stream 0)
+ * and is used primarily when managing prioritization.
*/
abstract class AbstractStream {
diff --git a/java/org/apache/coyote/http2/RecycledStream.java b/java/org/apache/coyote/http2/RecycledStream.java
new file mode 100644
index 0000000..f500646
--- /dev/null
+++ b/java/org/apache/coyote/http2/RecycledStream.java
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.coyote.http2;
+
+/**
+ * Represents a closed stream in the priority tree. Used in preference to the
+ * full {@link Stream} as has much lower memory usage.
+ */
+class RecycledStream extends AbstractNonZeroStream {
+
+ private final String connectionId;
+ private int weight;
+
+ RecycledStream(Stream stream) {
+ super(stream.getIdentifier());
+ connectionId = stream.getConnectionId();
+ weight = stream.getWeight();
+ }
+
+
+ @Override
+ String getConnectionId() {
+ return connectionId;
+ }
+
+
+ @Override
+ int getWeight() {
+ return weight;
+ }
+}
diff --git a/java/org/apache/coyote/http2/Stream.java b/java/org/apache/coyote/http2/Stream.java
index af777ab..56bc129 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -46,7 +46,7 @@ import org.apache.tomcat.util.net.ApplicationBufferHandler;
import org.apache.tomcat.util.net.WriteBuffer;
import org.apache.tomcat.util.res.StringManager;
-class Stream extends AbstractStream implements HeaderEmitter {
+class Stream extends AbstractNonZeroStream implements HeaderEmitter {
private static final Log log = LogFactory.getLog(Stream.class);
private static final StringManager sm = StringManager.getManager(Stream.class);
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 02/10: Reduce memory footprint of closed http/2 streams
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit e1a53fb2d567b81cfaa4fcfa94ef870f15a6e289
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Thu Sep 24 17:32:49 2020 +0100
Reduce memory footprint of closed http/2 streams
This refactoring replaces closed streams with a new RecycledStream
object and changes the mechanism used to look up known streams.
Pull up re-prioritisation
---
.../apache/coyote/http2/AbstractNonZeroStream.java | 71 ++++++++++++++++++++++
java/org/apache/coyote/http2/AbstractStream.java | 6 +-
.../apache/coyote/http2/Http2UpgradeHandler.java | 6 +-
java/org/apache/coyote/http2/RecycledStream.java | 10 +--
java/org/apache/coyote/http2/Stream.java | 55 -----------------
5 files changed, 78 insertions(+), 70 deletions(-)
diff --git a/java/org/apache/coyote/http2/AbstractNonZeroStream.java b/java/org/apache/coyote/http2/AbstractNonZeroStream.java
index 63fb5e7..ce08cc5 100644
--- a/java/org/apache/coyote/http2/AbstractNonZeroStream.java
+++ b/java/org/apache/coyote/http2/AbstractNonZeroStream.java
@@ -16,6 +16,11 @@
*/
package org.apache.coyote.http2;
+import java.util.Iterator;
+
+import org.apache.juli.logging.Log;
+import org.apache.juli.logging.LogFactory;
+import org.apache.tomcat.util.res.StringManager;
/**
* Base class for all streams other than stream 0, the connection. Primarily
@@ -23,7 +28,73 @@ package org.apache.coyote.http2;
*/
abstract class AbstractNonZeroStream extends AbstractStream {
+ private static final Log log = LogFactory.getLog(AbstractNonZeroStream.class);
+ private static final StringManager sm = StringManager.getManager(AbstractNonZeroStream.class);
+
+ private volatile int weight;
+
+
AbstractNonZeroStream(Integer identifier) {
+ this(identifier, Constants.DEFAULT_WEIGHT);
+ }
+
+
+ AbstractNonZeroStream(Integer identifier, int weight) {
super(identifier);
+ this.weight = weight;
+ }
+
+
+ @Override
+ final int getWeight() {
+ return weight;
}
+
+
+ final void rePrioritise(AbstractStream parent, boolean exclusive, int weight) {
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("stream.reprioritisation.debug",
+ getConnectionId(), getIdAsString(), Boolean.toString(exclusive),
+ parent.getIdAsString(), Integer.toString(weight)));
+ }
+
+ // Check if new parent is a descendant of this stream
+ if (isDescendant(parent)) {
+ parent.detachFromParent();
+ // Cast is always safe since any descendant of this stream must be
+ // an instance of Stream
+ getParentStream().addChild((Stream) parent);
+ }
+
+ if (exclusive) {
+ // Need to move children of the new parent to be children of this
+ // stream. Slightly convoluted to avoid concurrent modification.
+ Iterator<AbstractNonZeroStream> parentsChildren = parent.getChildStreams().iterator();
+ while (parentsChildren.hasNext()) {
+ AbstractNonZeroStream parentsChild = parentsChildren.next();
+ parentsChildren.remove();
+ this.addChild(parentsChild);
+ }
+ }
+ detachFromParent();
+ parent.addChild(this);
+ this.weight = weight;
+ }
+
+
+ /*
+ * Used when removing closed streams from the tree and we know there is no
+ * need to check for circular references.
+ */
+ final void rePrioritise(AbstractStream parent, int weight) {
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("stream.reprioritisation.debug",
+ getConnectionId(), getIdAsString(), Boolean.FALSE,
+ parent.getIdAsString(), Integer.toString(weight)));
+ }
+
+ parent.addChild(this);
+ this.weight = weight;
+ }
+
}
diff --git a/java/org/apache/coyote/http2/AbstractStream.java b/java/org/apache/coyote/http2/AbstractStream.java
index 21963ee..c7374b6 100644
--- a/java/org/apache/coyote/http2/AbstractStream.java
+++ b/java/org/apache/coyote/http2/AbstractStream.java
@@ -37,7 +37,7 @@ abstract class AbstractStream {
private final String idAsString;
private volatile AbstractStream parentStream = null;
- private final Set<Stream> childStreams = Collections.newSetFromMap(new ConcurrentHashMap<>());
+ private final Set<AbstractNonZeroStream> childStreams = Collections.newSetFromMap(new ConcurrentHashMap<>());
private long windowSize = ConnectionSettingsBase.DEFAULT_INITIAL_WINDOW_SIZE;
@@ -70,7 +70,7 @@ abstract class AbstractStream {
}
- final void addChild(Stream child) {
+ final void addChild(AbstractNonZeroStream child) {
child.setParentStream(this);
childStreams.add(child);
}
@@ -97,7 +97,7 @@ abstract class AbstractStream {
}
- final Set<Stream> getChildStreams() {
+ final Set<AbstractNonZeroStream> getChildStreams() {
return childStreams;
}
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index dbd7491..ad5e0e4 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -1269,17 +1269,17 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
Stream streamToRemove = streams.remove(streamIdToRemove);
// Move the removed Stream's children to the removed Stream's
// parent.
- Set<Stream> children = streamToRemove.getChildStreams();
+ Set<AbstractNonZeroStream> children = streamToRemove.getChildStreams();
if (children.size() == 1) {
// Shortcut
children.iterator().next().rePrioritise(
streamToRemove.getParentStream(), streamToRemove.getWeight());
} else {
int totalWeight = 0;
- for (Stream child : children) {
+ for (AbstractNonZeroStream child : children) {
totalWeight += child.getWeight();
}
- for (Stream child : children) {
+ for (AbstractNonZeroStream child : children) {
children.iterator().next().rePrioritise(
streamToRemove.getParentStream(),
streamToRemove.getWeight() * child.getWeight() / totalWeight);
diff --git a/java/org/apache/coyote/http2/RecycledStream.java b/java/org/apache/coyote/http2/RecycledStream.java
index f500646..1e630df 100644
--- a/java/org/apache/coyote/http2/RecycledStream.java
+++ b/java/org/apache/coyote/http2/RecycledStream.java
@@ -23,12 +23,10 @@ package org.apache.coyote.http2;
class RecycledStream extends AbstractNonZeroStream {
private final String connectionId;
- private int weight;
RecycledStream(Stream stream) {
- super(stream.getIdentifier());
+ super(stream.getIdentifier(), stream.getWeight());
connectionId = stream.getConnectionId();
- weight = stream.getWeight();
}
@@ -36,10 +34,4 @@ class RecycledStream extends AbstractNonZeroStream {
String getConnectionId() {
return connectionId;
}
-
-
- @Override
- int getWeight() {
- return weight;
- }
}
diff --git a/java/org/apache/coyote/http2/Stream.java b/java/org/apache/coyote/http2/Stream.java
index 56bc129..ef7ed29 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -23,7 +23,6 @@ import java.security.AccessController;
import java.security.PrivilegedActionException;
import java.security.PrivilegedExceptionAction;
import java.util.Collections;
-import java.util.Iterator;
import java.util.Locale;
import java.util.Map;
import java.util.function.Supplier;
@@ -67,7 +66,6 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter {
ACK_HEADERS = response.getMimeHeaders();
}
- private volatile int weight = Constants.DEFAULT_WEIGHT;
private volatile long contentLengthReceived = 0;
private final Http2UpgradeHandler handler;
@@ -175,53 +173,6 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter {
}
- final void rePrioritise(AbstractStream parent, boolean exclusive, int weight) {
- if (log.isDebugEnabled()) {
- log.debug(sm.getString("stream.reprioritisation.debug",
- getConnectionId(), getIdAsString(), Boolean.toString(exclusive),
- parent.getIdAsString(), Integer.toString(weight)));
- }
-
- // Check if new parent is a descendant of this stream
- if (isDescendant(parent)) {
- parent.detachFromParent();
- // Cast is always safe since any descendant of this stream must be
- // an instance of Stream
- getParentStream().addChild((Stream) parent);
- }
-
- if (exclusive) {
- // Need to move children of the new parent to be children of this
- // stream. Slightly convoluted to avoid concurrent modification.
- Iterator<Stream> parentsChildren = parent.getChildStreams().iterator();
- while (parentsChildren.hasNext()) {
- Stream parentsChild = parentsChildren.next();
- parentsChildren.remove();
- this.addChild(parentsChild);
- }
- }
- detachFromParent();
- parent.addChild(this);
- this.weight = weight;
- }
-
-
- /*
- * Used when removing closed streams from the tree and we know there is no
- * need to check for circular references.
- */
- final void rePrioritise(AbstractStream parent, int weight) {
- if (log.isDebugEnabled()) {
- log.debug(sm.getString("stream.reprioritisation.debug",
- getConnectionId(), getIdAsString(), Boolean.FALSE,
- parent.getIdAsString(), Integer.toString(weight)));
- }
-
- parent.addChild(this);
- this.weight = weight;
- }
-
-
final void receiveReset(long errorCode) {
if (log.isDebugEnabled()) {
log.debug(sm.getString("stream.reset.receive", getConnectionId(), getIdAsString(),
@@ -566,12 +517,6 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter {
}
- @Override
- final int getWeight() {
- return weight;
- }
-
-
final Request getCoyoteRequest() {
return coyoteRequest;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 06/10: Reduce memory footprint of closed http/2 streams
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 2b2ac4f3ce99027d8589595e921570495f7b08c0
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Thu Sep 24 21:47:09 2020 +0100
Reduce memory footprint of closed http/2 streams
This refactoring replaces closed streams with a new RecycledStream
object and changes the mechanism used to look up known streams.
Pull up state
---
.../apache/coyote/http2/AbstractNonZeroStream.java | 21 ++++++++++++++++-----
java/org/apache/coyote/http2/RecycledStream.java | 16 +---------------
java/org/apache/coyote/http2/Stream.java | 16 +---------------
3 files changed, 18 insertions(+), 35 deletions(-)
diff --git a/java/org/apache/coyote/http2/AbstractNonZeroStream.java b/java/org/apache/coyote/http2/AbstractNonZeroStream.java
index 582ab1e..084de33 100644
--- a/java/org/apache/coyote/http2/AbstractNonZeroStream.java
+++ b/java/org/apache/coyote/http2/AbstractNonZeroStream.java
@@ -31,17 +31,22 @@ abstract class AbstractNonZeroStream extends AbstractStream {
private static final Log log = LogFactory.getLog(AbstractNonZeroStream.class);
private static final StringManager sm = StringManager.getManager(AbstractNonZeroStream.class);
+ protected final StreamStateMachine state;
+
private volatile int weight;
- AbstractNonZeroStream(Integer identifier) {
- this(identifier, 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) {
+ AbstractNonZeroStream(Integer identifier, int weight, StreamStateMachine state) {
super(identifier);
this.weight = weight;
+ this.state = state;
}
@@ -97,7 +102,13 @@ abstract class AbstractNonZeroStream extends AbstractStream {
this.weight = weight;
}
- abstract boolean isClosedFinal();
- abstract void checkState(FrameType frameType) throws Http2Exception;
+ final boolean isClosedFinal() {
+ return state.isClosedFinal();
+ }
+
+
+ final void checkState(FrameType frameType) throws Http2Exception {
+ state.checkFrameType(frameType);
+ }
}
diff --git a/java/org/apache/coyote/http2/RecycledStream.java b/java/org/apache/coyote/http2/RecycledStream.java
index 1915dff..9d6177c 100644
--- a/java/org/apache/coyote/http2/RecycledStream.java
+++ b/java/org/apache/coyote/http2/RecycledStream.java
@@ -23,12 +23,10 @@ package org.apache.coyote.http2;
class RecycledStream extends AbstractNonZeroStream {
private final String connectionId;
- private final StreamStateMachine state;
RecycledStream(String connectionId, Integer identifier, int weight, StreamStateMachine state) {
- super(identifier, weight);
+ super(identifier, weight, state);
this.connectionId = connectionId;
- this.state = state;
}
@@ -38,18 +36,6 @@ class RecycledStream extends AbstractNonZeroStream {
}
- @Override
- boolean isClosedFinal() {
- return state.isClosedFinal();
- }
-
-
- @Override
- final void checkState(FrameType frameType) throws Http2Exception {
- state.checkFrameType(frameType);
- }
-
-
@SuppressWarnings("sync-override")
@Override
void incrementWindowSize(int increment) throws Http2Exception {
diff --git a/java/org/apache/coyote/http2/Stream.java b/java/org/apache/coyote/http2/Stream.java
index f6210d7..140a76d 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -69,7 +69,6 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter {
private volatile long contentLengthReceived = 0;
private final Http2UpgradeHandler handler;
- private final StreamStateMachine state;
private final WindowAllocationManager allocationManager = new WindowAllocationManager(this);
// State machine would be too much overhead
@@ -93,11 +92,10 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter {
Stream(Integer identifier, Http2UpgradeHandler handler, Request coyoteRequest) {
- super(identifier);
+ super(handler.getConnectionId(), identifier);
this.handler = handler;
handler.addChild(this);
setWindowSize(handler.getRemoteSettings().getInitialWindowSize());
- state = new StreamStateMachine(getConnectionId(), getIdAsString());
if (coyoteRequest == null) {
// HTTP/2 new request
this.coyoteRequest = new Request();
@@ -194,12 +192,6 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter {
@Override
- final void checkState(FrameType frameType) throws Http2Exception {
- state.checkFrameType(frameType);
- }
-
-
- @Override
final synchronized void incrementWindowSize(int windowSizeIncrement) throws Http2Exception {
// If this is zero then any thread that has been trying to write for
// this stream will be waiting. Notify that thread it can continue. Use
@@ -647,12 +639,6 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter {
}
- @Override
- final boolean isClosedFinal() {
- return state.isClosedFinal();
- }
-
-
final void closeIfIdle() {
state.closeIfIdle();
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 08/10: Refactor the pruning so more stream info is
retained for longer
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 83f5e03b52c54a9b213e979c9bb47e0967530cbe
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 aebb70b..9f71af9 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<>();
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 04/10: Reduce memory footprint of closed http/2 streams
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 496b3cf5dd99179cd5460c728819bd799f662f99
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Thu Sep 24 18:11:37 2020 +0100
Reduce memory footprint of closed http/2 streams
This refactoring replaces closed streams with a new RecycledStream
object and changes the mechanism used to look up known streams.
Refactoring getStream to handle differences between Stream and
RecycledStream
---
.../apache/coyote/http2/AbstractNonZeroStream.java | 2 +
.../apache/coyote/http2/Http2UpgradeHandler.java | 125 ++++++++++++++-------
java/org/apache/coyote/http2/RecycledStream.java | 26 ++++-
java/org/apache/coyote/http2/Stream.java | 20 ++--
4 files changed, 111 insertions(+), 62 deletions(-)
diff --git a/java/org/apache/coyote/http2/AbstractNonZeroStream.java b/java/org/apache/coyote/http2/AbstractNonZeroStream.java
index 70cfe35..582ab1e 100644
--- a/java/org/apache/coyote/http2/AbstractNonZeroStream.java
+++ b/java/org/apache/coyote/http2/AbstractNonZeroStream.java
@@ -98,4 +98,6 @@ abstract class AbstractNonZeroStream extends AbstractStream {
}
abstract boolean isClosedFinal();
+
+ abstract void checkState(FrameType frameType) throws Http2Exception;
}
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index ad5e0e4..c51efd9 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -29,6 +29,7 @@ import java.util.Set;
import java.util.TreeSet;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicReference;
@@ -121,7 +122,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
private HpackDecoder hpackDecoder;
private HpackEncoder hpackEncoder;
- private final Map<Integer,Stream> streams = new ConcurrentHashMap<>();
+ private final ConcurrentMap<Integer,AbstractNonZeroStream> streams = new ConcurrentHashMap<>();
protected final AtomicInteger activeRemoteStreamCount = new AtomicInteger(0);
// Start at -1 so the 'add 2' logic in closeIdleStreams() works
private volatile int maxActiveRemoteStreamId = -1;
@@ -1089,7 +1090,22 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
private Stream getStream(int streamId, boolean unknownIsError) throws ConnectionException {
Integer key = Integer.valueOf(streamId);
- Stream result = streams.get(key);
+ AbstractStream result = streams.get(key);
+ if (result instanceof Stream) {
+ return (Stream) result;
+ }
+ if (unknownIsError) {
+ // Stream has been closed and removed from the map
+ throw new ConnectionException(sm.getString("upgradeHandler.stream.closed", key.toString()),
+ Http2Error.PROTOCOL_ERROR);
+ }
+ return null;
+ }
+
+
+ private AbstractNonZeroStream getStreamMayBeClosed(int streamId, boolean unknownIsError) throws ConnectionException {
+ Integer key = Integer.valueOf(streamId);
+ AbstractNonZeroStream result = streams.get(key);
if (result == null && unknownIsError) {
// Stream has been closed and removed from the map
throw new ConnectionException(sm.getString("upgradeHandler.stream.closed", key.toString()),
@@ -1133,10 +1149,12 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
return;
}
- for (Stream stream : streams.values()) {
- // The connection is closing. Close the associated streams as no
- // longer required (also notifies any threads waiting for allocations).
- stream.receiveReset(Http2Error.CANCEL.getCode());
+ for (AbstractNonZeroStream stream : streams.values()) {
+ if (stream instanceof Stream) {
+ // The connection is closing. Close the associated streams as no
+ // longer required (also notifies any threads waiting for allocations).
+ ((Stream) stream).receiveReset(Http2Error.CANCEL.getCode());
+ }
}
try {
socketWrapper.close();
@@ -1193,10 +1211,10 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
TreeSet<Integer> candidatesStepTwo = new TreeSet<>();
TreeSet<Integer> candidatesStepThree = new TreeSet<>();
- for (Entry<Integer, Stream> entry : streams.entrySet()) {
- Stream stream = entry.getValue();
+ for (Entry<Integer, AbstractNonZeroStream> entry : streams.entrySet()) {
+ AbstractNonZeroStream stream = entry.getValue();
// Never remove active streams
- if (stream.isActive()) {
+ if (stream instanceof Stream && ((Stream) stream).isActive()) {
continue;
}
@@ -1217,7 +1235,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
// Process the step one list
for (Integer streamIdToRemove : candidatesStepOne) {
// Remove this childless stream
- Stream removedStream = streams.remove(streamIdToRemove);
+ AbstractNonZeroStream removedStream = streams.remove(streamIdToRemove);
removedStream.detachFromParent();
toClose--;
if (log.isDebugEnabled()) {
@@ -1266,7 +1284,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
private void removeStreamFromPriorityTree(Integer streamIdToRemove) {
- Stream streamToRemove = streams.remove(streamIdToRemove);
+ AbstractNonZeroStream streamToRemove = streams.remove(streamIdToRemove);
// Move the removed Stream's children to the removed Stream's
// parent.
Set<AbstractNonZeroStream> children = streamToRemove.getChildStreams();
@@ -1415,24 +1433,33 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
}
}
- Stream stream = getStream(streamId, true);
- stream.checkState(FrameType.DATA);
- stream.receivedData(payloadSize);
- return stream.getInputByteBuffer();
+ AbstractNonZeroStream abstractNonZeroStream = getStreamMayBeClosed(streamId, true);
+ if (abstractNonZeroStream instanceof Stream) {
+ Stream stream = (Stream) abstractNonZeroStream;
+ stream.checkState(FrameType.DATA);
+ stream.receivedData(payloadSize);
+ return stream.getInputByteBuffer();
+ } else {
+ abstractNonZeroStream.checkState(FrameType.DATA);
+ return null;
+ }
}
@Override
public void endRequestBodyFrame(int streamId) throws Http2Exception {
- Stream stream = getStream(streamId, true);
- stream.getInputBuffer().onDataAvailable();
+ AbstractNonZeroStream abstractNonZeroStream = getStreamMayBeClosed(streamId, true);
+ if (abstractNonZeroStream instanceof Stream) {
+ ((Stream) abstractNonZeroStream).getInputBuffer().onDataAvailable();
+ }
}
@Override
public void receivedEndOfStream(int streamId) throws ConnectionException {
- Stream stream = getStream(streamId, connectionState.get().isNewStreamAllowed());
- if (stream != null) {
+ AbstractNonZeroStream abstractNonZeroStream = getStreamMayBeClosed(streamId, connectionState.get().isNewStreamAllowed());
+ if (abstractNonZeroStream instanceof Stream) {
+ Stream stream = (Stream) abstractNonZeroStream;
stream.receivedEndOfStream();
if (!stream.isActive()) {
setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet());
@@ -1444,9 +1471,11 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
@Override
public void swallowedPadding(int streamId, int paddingLength) throws
ConnectionException, IOException {
- Stream stream = getStream(streamId, true);
- // +1 is for the payload byte used to define the padding length
- writeWindowUpdate(stream, paddingLength + 1, false);
+ AbstractNonZeroStream abstractNonZeroStream = getStreamMayBeClosed(streamId, true);
+ if (abstractNonZeroStream instanceof Stream) {
+ // +1 is for the payload byte used to define the padding length
+ writeWindowUpdate((Stream) abstractNonZeroStream, paddingLength + 1, false);
+ }
}
@@ -1485,10 +1514,12 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
private void closeIdleStreams(int newMaxActiveRemoteStreamId) {
- for (Entry<Integer,Stream> entry : streams.entrySet()) {
- if (entry.getKey().intValue() > maxActiveRemoteStreamId &&
- entry.getKey().intValue() < newMaxActiveRemoteStreamId) {
- entry.getValue().closeIfIdle();
+ for (Entry<Integer,AbstractNonZeroStream> entry : streams.entrySet()) {
+ int id = entry.getKey().intValue();
+ if (id > maxActiveRemoteStreamId && id < newMaxActiveRemoteStreamId) {
+ if (entry.getValue() instanceof Stream) {
+ ((Stream) entry.getValue()).closeIfIdle();
+ }
}
}
maxActiveRemoteStreamId = newMaxActiveRemoteStreamId;
@@ -1505,16 +1536,15 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
increaseOverheadCount();
- Stream stream = getStream(streamId, false);
- if (stream == null) {
- stream = createRemoteStream(streamId);
+ AbstractNonZeroStream abstractNonZeroStream = getStreamMayBeClosed(streamId, false);
+ if (abstractNonZeroStream == null) {
+ abstractNonZeroStream = createRemoteStream(streamId);
}
- stream.checkState(FrameType.PRIORITY);
- AbstractStream parentStream = getStream(parentStreamId, false);
+ AbstractStream parentStream = getStreamMayBeClosed(parentStreamId, false);
if (parentStream == null) {
parentStream = this;
}
- stream.rePrioritise(parentStream, exclusive, weight);
+ abstractNonZeroStream.rePrioritise(parentStream, exclusive, weight);
}
@@ -1539,9 +1569,10 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
@Override
public void headersEnd(int streamId) throws Http2Exception {
- Stream stream = getStream(streamId, connectionState.get().isNewStreamAllowed());
- if (stream != null) {
+ AbstractNonZeroStream abstractNonZeroStream = getStreamMayBeClosed(streamId, connectionState.get().isNewStreamAllowed());
+ if (abstractNonZeroStream instanceof Stream) {
setMaxProcessedStream(streamId);
+ Stream stream = (Stream) abstractNonZeroStream;
if (stream.isActive()) {
if (stream.receivedEndOfHeaders()) {
@@ -1576,12 +1607,15 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
log.debug(sm.getString("upgradeHandler.reset.receive", getConnectionId(), Integer.toString(streamId),
Long.toString(errorCode)));
}
- Stream stream = getStream(streamId, true);
- boolean active = stream.isActive();
- stream.checkState(FrameType.RST);
- stream.receiveReset(errorCode);
- if (active) {
- activeRemoteStreamCount.decrementAndGet();
+ AbstractNonZeroStream abstractNonZeroStream = getStreamMayBeClosed(streamId, true);
+ abstractNonZeroStream.checkState(FrameType.RST);
+ if (abstractNonZeroStream instanceof Stream) {
+ Stream stream = (Stream) abstractNonZeroStream;
+ boolean active = stream.isActive();
+ stream.receiveReset(errorCode);
+ if (active) {
+ activeRemoteStreamCount.decrementAndGet();
+ }
}
}
@@ -1602,11 +1636,11 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
// Do this first in case new value is invalid
remoteSettings.set(setting, value);
int diff = (int) (value - oldValue);
- for (Stream stream : streams.values()) {
+ for (AbstractNonZeroStream stream : streams.values()) {
try {
stream.incrementWindowSize(diff);
} catch (Http2Exception h2e) {
- stream.close(new StreamException(sm.getString(
+ ((Stream) stream).close(new StreamException(sm.getString(
"upgradeHandler.windowSizeTooBig", connectionId,
stream.getIdAsString()),
h2e.getError(), stream.getIdAsInt()));
@@ -1679,7 +1713,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
incrementWindowSize(increment);
} else {
- Stream stream = getStream(streamId, true);
+ AbstractNonZeroStream stream = getStreamMayBeClosed(streamId, true);
// Check for small increments which are inefficient
if (average < overheadThreshold) {
@@ -1705,6 +1739,11 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
}
+ void replaceStream(AbstractNonZeroStream original, AbstractNonZeroStream replacement) {
+ streams.replace(original.getIdentifier(), replacement);
+ }
+
+
protected class PingManager {
protected boolean initiateDisabled = false;
diff --git a/java/org/apache/coyote/http2/RecycledStream.java b/java/org/apache/coyote/http2/RecycledStream.java
index dbbdc10..1915dff 100644
--- a/java/org/apache/coyote/http2/RecycledStream.java
+++ b/java/org/apache/coyote/http2/RecycledStream.java
@@ -23,12 +23,12 @@ package org.apache.coyote.http2;
class RecycledStream extends AbstractNonZeroStream {
private final String connectionId;
- private final boolean closedFinal;
+ private final StreamStateMachine state;
- RecycledStream(Stream stream) {
- super(stream.getIdentifier(), stream.getWeight());
- connectionId = stream.getConnectionId();
- closedFinal = stream.isClosedFinal();
+ RecycledStream(String connectionId, Integer identifier, int weight, StreamStateMachine state) {
+ super(identifier, weight);
+ this.connectionId = connectionId;
+ this.state = state;
}
@@ -40,6 +40,20 @@ class RecycledStream extends AbstractNonZeroStream {
@Override
boolean isClosedFinal() {
- return closedFinal;
+ return state.isClosedFinal();
+ }
+
+
+ @Override
+ final void checkState(FrameType frameType) throws Http2Exception {
+ state.checkFrameType(frameType);
+ }
+
+
+ @SuppressWarnings("sync-override")
+ @Override
+ void incrementWindowSize(int increment) throws Http2Exception {
+ // NO-OP
}
}
+
diff --git a/java/org/apache/coyote/http2/Stream.java b/java/org/apache/coyote/http2/Stream.java
index 489ce53..8aa4a7b 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -193,6 +193,7 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter {
}
+ @Override
final void checkState(FrameType frameType) throws Http2Exception {
state.checkFrameType(frameType);
}
@@ -688,24 +689,17 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter {
/*
* This method is called recycle for consistency with the rest of the Tomcat
- * code base. Currently, it only sets references to null for the purposes of
- * reducing memory footprint. It does not fully recycle the Stream ready for
- * re-use since Stream objects are not re-used. This is useful because
- * Stream instances are retained for a period after the Stream closes.
+ * code base. Currently, it calls the handler to replace this stream with an
+ * implementation that uses less memory. It does not fully recycle the
+ * Stream ready for re-use since Stream objects are not re-used. This is
+ * useful because Stream instances are retained for a period after the
+ * Stream closes.
*/
final void recycle() {
if (log.isDebugEnabled()) {
log.debug(sm.getString("stream.recycle", getConnectionId(), getIdAsString()));
}
- /*
- * Temporarily disabled due to multiple regressions (NPEs)
- coyoteRequest = null;
- cookieHeader = null;
- coyoteResponse = null;
- inputBuffer = null;
- streamOutputBuffer = null;
- http2OutputBuffer = null;
- */
+ handler.replaceStream(this, new RecycledStream(getConnectionId(), getIdentifier(), getWeight(), state));
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 07/10: Fully replace Stream with RecycledStream
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit d1f204ee12ed3aeb7713b95661a53ddaadd7e9b7
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 084de33..bbdfa33 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 c51efd9..aebb70b 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -94,6 +94,8 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
private static final HeaderSink HEADER_SINK = new HeaderSink();
+ private final Object priorityTreeLock = new Object();
+
protected final String connectionId;
protected final Http2Protocol protocol;
@@ -104,8 +106,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
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;
/**
@@ -1175,7 +1176,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
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();
final int size = streams.size();
@@ -1184,9 +1185,13 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
Long.toString(max), Integer.toString(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;
}
@@ -1284,27 +1289,29 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
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
- children.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();
- children.clear();
}
@@ -1544,7 +1551,9 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
if (parentStream == null) {
parentStream = this;
}
- abstractNonZeroStream.rePrioritise(parentStream, exclusive, weight);
+ synchronized (priorityTreeLock) {
+ abstractNonZeroStream.rePrioritise(parentStream, exclusive, weight);
+ }
}
@@ -1740,7 +1749,10 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
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 9d6177c..b2b3e32 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 140a76d..982eae0 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -685,7 +685,7 @@ 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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 09/10: Update changelog
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit a4054eb3f27ac0d242d5f2a6c4599e3f6cb9a519
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Fri Sep 25 14:33:45 2020 +0100
Update changelog
---
webapps/docs/changelog.xml | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 0be3539..a82ec7b 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -127,6 +127,11 @@
(markt)
</fix>
<fix>
+ Refactor the handling of closed HTTP/2 streams to reduce the heap usage
+ associated with used streams and to retain information for more streams
+ in the priority tree. (markt)
+ </fix>
+ <fix>
Don't send the Keep-Alive response header if the connection has been
explicitly closed. (markt)
</fix>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org