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/02/17 15:31:58 UTC
[tomcat] branch master updated: Refactor doOptions() to improve
performance
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
The following commit(s) were added to refs/heads/master by this push:
new 94cdbc3 Refactor doOptions() to improve performance
94cdbc3 is described below
commit 94cdbc3286765d0a86a07ee1268f5f03f9579478
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon Feb 17 14:10:43 2020 +0000
Refactor doOptions() to improve performance
Move the Tomcat specific hack to an inner class and do most of the
reflection once on first use rather than on every doOptions() call.
Cache the constant part of the allow value for options on first request
For 10,000,000 iterations this represents ~70x improvement.
As expected improvement is smaller for fewer iterations although it
is measureable from ~10 iterations upwards.
At very low (<5) iterations there is ~10% additional overhead.
Note: Users that really care about performance will implement their
own doOptions() method
---
java/jakarta/servlet/http/HttpServlet.java | 186 ++++++++++++++-------
.../servlet/http/TestHttpServletPerformance.java | 70 ++++++++
webapps/docs/changelog.xml | 8 +
3 files changed, 207 insertions(+), 57 deletions(-)
diff --git a/java/jakarta/servlet/http/HttpServlet.java b/java/jakarta/servlet/http/HttpServlet.java
index fff8468..56b4ce1 100644
--- a/java/jakarta/servlet/http/HttpServlet.java
+++ b/java/jakarta/servlet/http/HttpServlet.java
@@ -92,6 +92,9 @@ public abstract class HttpServlet extends GenericServlet {
private static final ResourceBundle lStrings =
ResourceBundle.getBundle(LSTRING_FILE);
+ private final Object cachedAllowHeaderValueLock = new Object();
+ private volatile String cachedAllowHeaderValue = null;
+
/**
* Does nothing, because this is an abstract class.
@@ -421,6 +424,89 @@ public abstract class HttpServlet extends GenericServlet {
}
+ private String getCachedAllowHeaderValue() {
+ if (cachedAllowHeaderValue == null) {
+ synchronized (cachedAllowHeaderValueLock) {
+ if (cachedAllowHeaderValue == null) {
+
+ Method[] methods = getAllDeclaredMethods(this.getClass());
+
+ // RFC 7230 does not define an order for this header
+ // This code aims to retain, broadly, the order of method
+ // tokens returned in earlier versions of this code. If that
+ // constraint is dropped then the code can be simplified
+ // further.
+
+ boolean allowGet = false;
+ boolean allowHead = false;
+ boolean allowPost = false;
+ boolean allowPut = false;
+ boolean allowDelete = false;
+
+ for (int i = 0; i < methods.length; i++) {
+ switch (methods[i].getName()) {
+ case "doGet": {
+ allowGet = true;
+ allowHead = true;
+ break;
+ }
+ case "doPost": {
+ allowPost = true;
+ break;
+ }
+ case "doPut": {
+ allowPut = true;
+ break;
+ }
+ case "doDelete": {
+ allowDelete = true;
+ break;
+ }
+ default:
+ // NO-OP
+ }
+
+ }
+
+ StringBuilder allow = new StringBuilder();
+
+ if (allowGet) {
+ allow.append(METHOD_GET);
+ allow.append(", ");
+ }
+
+ if (allowHead) {
+ allow.append(METHOD_HEAD);
+ allow.append(", ");
+ }
+
+ if (allowPost) {
+ allow.append(METHOD_POST);
+ allow.append(", ");
+ }
+
+ if (allowPut) {
+ allow.append(METHOD_PUT);
+ allow.append(", ");
+ }
+
+ if (allowDelete) {
+ allow.append(METHOD_DELETE);
+ allow.append(", ");
+ }
+
+ // Options is always allowed
+ allow.append(METHOD_OPTIONS);
+
+ cachedAllowHeaderValue = allow.toString();
+ }
+ }
+ }
+
+ return cachedAllowHeaderValue;
+ }
+
+
private static Method[] getAllDeclaredMethods(Class<?> c) {
if (c.equals(jakarta.servlet.http.HttpServlet.class)) {
@@ -476,69 +562,20 @@ public abstract class HttpServlet extends GenericServlet {
* @exception ServletException if the request for the
* OPTIONS cannot be handled
*/
- protected void doOptions(HttpServletRequest req,
- HttpServletResponse resp)
- throws ServletException, IOException {
+ protected void doOptions(HttpServletRequest req, HttpServletResponse resp)
+ throws ServletException, IOException {
- Method[] methods = getAllDeclaredMethods(this.getClass());
-
- boolean ALLOW_GET = false;
- boolean ALLOW_HEAD = false;
- boolean ALLOW_POST = false;
- boolean ALLOW_PUT = false;
- boolean ALLOW_DELETE = false;
- boolean ALLOW_TRACE = true;
- boolean ALLOW_OPTIONS = true;
+ String allow = getCachedAllowHeaderValue();
// Tomcat specific hack to see if TRACE is allowed
- Class<?> clazz = null;
- try {
- clazz = Class.forName("org.apache.catalina.connector.RequestFacade");
- Method getAllowTrace = clazz.getMethod("getAllowTrace", (Class<?>[]) null);
- ALLOW_TRACE = ((Boolean) getAllowTrace.invoke(req, (Object[]) null)).booleanValue();
- } catch (ClassNotFoundException | NoSuchMethodException | SecurityException |
- IllegalAccessException | IllegalArgumentException | InvocationTargetException e) {
- // Ignore. Not running on Tomcat. TRACE is always allowed.
- }
- // End of Tomcat specific hack
-
- for (int i=0; i<methods.length; i++) {
- Method m = methods[i];
-
- if (m.getName().equals("doGet")) {
- ALLOW_GET = true;
- ALLOW_HEAD = true;
+ if (TomcatHack.getAllowTrace(req)) {
+ if (allow.length() == 0) {
+ allow = METHOD_TRACE;
+ } else {
+ allow = allow + ", " + METHOD_TRACE;
}
- if (m.getName().equals("doPost"))
- ALLOW_POST = true;
- if (m.getName().equals("doPut"))
- ALLOW_PUT = true;
- if (m.getName().equals("doDelete"))
- ALLOW_DELETE = true;
}
- String allow = null;
- if (ALLOW_GET)
- allow=METHOD_GET;
- if (ALLOW_HEAD)
- if (allow==null) allow=METHOD_HEAD;
- else allow += ", " + METHOD_HEAD;
- if (ALLOW_POST)
- if (allow==null) allow=METHOD_POST;
- else allow += ", " + METHOD_POST;
- if (ALLOW_PUT)
- if (allow==null) allow=METHOD_PUT;
- else allow += ", " + METHOD_PUT;
- if (ALLOW_DELETE)
- if (allow==null) allow=METHOD_DELETE;
- else allow += ", " + METHOD_DELETE;
- if (ALLOW_TRACE)
- if (allow==null) allow=METHOD_TRACE;
- else allow += ", " + METHOD_TRACE;
- if (ALLOW_OPTIONS)
- if (allow==null) allow=METHOD_OPTIONS;
- else allow += ", " + METHOD_OPTIONS;
-
resp.setHeader("Allow", allow);
}
@@ -740,6 +777,41 @@ public abstract class HttpServlet extends GenericServlet {
}
service(request, response);
}
+
+
+ private static class TomcatHack {
+
+ private static final Class<?> REQUEST_FACADE_CLAZZ;
+ private static final Method GET_ALLOW_TRACE;
+
+
+ static {
+ Method m1 = null;
+ Class<?> c1 = null;
+ try {
+ c1 = Class.forName("org.apache.catalina.connector.RequestFacade");
+ m1 = c1.getMethod("getAllowTrace", (Class<?>[]) null);
+ } catch (ReflectiveOperationException | SecurityException | IllegalArgumentException e) {
+ // Ignore. Not running on Tomcat. TRACE is always allowed.
+ }
+ REQUEST_FACADE_CLAZZ = c1;
+ GET_ALLOW_TRACE = m1;
+ }
+
+ public static boolean getAllowTrace(HttpServletRequest req) {
+ if (REQUEST_FACADE_CLAZZ != null && GET_ALLOW_TRACE != null) {
+ if (REQUEST_FACADE_CLAZZ.isAssignableFrom(req.getClass())) {
+ try {
+ return ((Boolean) GET_ALLOW_TRACE.invoke(req, (Object[]) null)).booleanValue();
+ } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) {
+ // Should never happen given the checks in place.
+ // Ignore
+ }
+ }
+ }
+ return true;
+ }
+ }
}
diff --git a/test/jakarta/servlet/http/TestHttpServletPerformance.java b/test/jakarta/servlet/http/TestHttpServletPerformance.java
new file mode 100644
index 0000000..0a8de69
--- /dev/null
+++ b/test/jakarta/servlet/http/TestHttpServletPerformance.java
@@ -0,0 +1,70 @@
+/*
+ * 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 jakarta.servlet.http;
+
+import java.io.IOException;
+
+import jakarta.servlet.ServletException;
+
+import org.junit.Test;
+
+import org.apache.catalina.connector.RequestFacade;
+import org.apache.catalina.filters.TesterHttpServletResponse;
+
+
+/*
+ * Note: Class name chosen so it matches *Performance.java and can be excluded
+ * from test runs along with other performance tests.
+ */
+public class TestHttpServletPerformance {
+
+ @Test
+ public void testDoOptions() throws IOException, ServletException{
+ TesterServlet testerServlet = new TesterServlet();
+ TesterRequest testerRequest = new TesterRequest(false);
+ TesterHttpServletResponse testerResponse = new TesterHttpServletResponse();
+
+ long start = System.nanoTime();
+ for (int i = 0; i < 10000000; i++) {
+ testerServlet.doOptions(testerRequest, testerResponse);
+ }
+ long end = System.nanoTime();
+
+ System.out.println("doOptions()" + (end - start) + "ns");
+ }
+
+
+ private static class TesterServlet extends HttpServlet {
+ private static final long serialVersionUID = 1L;
+ }
+
+
+ private static class TesterRequest extends RequestFacade {
+
+ private final boolean allowTrace;
+
+ public TesterRequest(boolean allowTrace) {
+ super(null);
+ this.allowTrace = allowTrace;
+ }
+
+ @Override
+ public boolean getAllowTrace() {
+ return allowTrace;
+ }
+ }
+}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 87fd5be..d22d40f 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -45,6 +45,14 @@
issues do not "pop up" wrt. others).
-->
<section name="Tomcat 10.0.0-M2 (markt)" rtext="in development">
+ <subsection name="Catalina">
+ <changelog>
+ <scode>
+ Refactor <code>HttpServlet.doOptions()</code> to improve performance.
+ (markt)
+ </scode>
+ </changelog>
+ </subsection>
<subsection name="Coyote">
<changelog>
<fix>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org