diff --git a/instrumentation/benchmarks/src/main/java/brave/servlet/ServletRuntimeBenchmarks.java b/instrumentation/benchmarks/src/main/java/brave/servlet/ServletRuntimeBenchmarks.java new file mode 100644 index 0000000000..556463f7d5 --- /dev/null +++ b/instrumentation/benchmarks/src/main/java/brave/servlet/ServletRuntimeBenchmarks.java @@ -0,0 +1,289 @@ +/** + * Copyright 2015-2016 The OpenZipkin Authors + * + * Licensed 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 brave.servlet; + +import java.io.PrintWriter; +import java.util.Collection; +import java.util.Locale; +import java.util.concurrent.TimeUnit; +import javax.servlet.ServletOutputStream; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletResponse; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Group; +import org.openjdk.jmh.annotations.GroupThreads; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.RunnerException; +import org.openjdk.jmh.runner.options.Options; +import org.openjdk.jmh.runner.options.OptionsBuilder; + +@Measurement(iterations = 5, time = 1) +@Warmup(iterations = 10, time = 1) +@Fork(3) +@BenchmarkMode(Mode.Throughput) +@OutputTimeUnit(TimeUnit.MICROSECONDS) +public class ServletRuntimeBenchmarks { + static final ServletRuntime servlet3 = new ServletRuntime.Servlet3(); + static final ServletRuntime servlet25 = new ServletRuntime.Servlet25(); + + @Benchmark @Group("no_contention") @GroupThreads(1) + public int no_contention_status_servlet3() { + return threeStatuses(servlet3); + } + + @Benchmark @Group("mild_contention") @GroupThreads(2) + public int mild_contention_status_servlet3() { + return threeStatuses(servlet3); + } + + @Benchmark @Group("high_contention") @GroupThreads(8) + public int high_contention_status_servlet3() { + return threeStatuses(servlet3); + } + + int threeStatuses(ServletRuntime runtime) { + // & to ensure null wasn't returned (forces NPE) + return runtime.status(new Response1()) & + runtime.status(new Response2()) & + runtime.status(new Response3()); + } + + @Benchmark @Group("no_contention") @GroupThreads(1) + public int no_contention_httpResponse_servlet3() { + return threeResponses(servlet3); + } + + @Benchmark @Group("mild_contention") @GroupThreads(2) + public int mild_contention_httpResponse_servlet3() { + return threeResponses(servlet3); + } + + @Benchmark @Group("high_contention") @GroupThreads(8) + public int high_contention_httpResponse_servlet3() { + return threeResponses(servlet3); + } + + int threeResponses(ServletRuntime runtime) { + // & to ensure null wasn't returned (forces NPE) + return runtime.httpResponse(new Response1()).getStatus() & + runtime.httpResponse(new Response2()).getStatus() & + runtime.httpResponse(new Response3()).getStatus(); + } + + @Benchmark @Group("no_contention") @GroupThreads(1) + public int no_contention_status_servlet25() { + return threeStatuses(servlet25); + } + + @Benchmark @Group("mild_contention") @GroupThreads(2) + public int mild_contention_status_servlet25() { + return threeStatuses(servlet25); + } + + @Benchmark @Group("high_contention") @GroupThreads(8) + public int high_contention_status_servlet25() { + return threeStatuses(servlet25); + } + + @Benchmark @Group("no_contention") @GroupThreads(1) + public int no_contention_httpResponse_servlet25() { + return threeResponses(servlet25); + } + + @Benchmark @Group("mild_contention") @GroupThreads(2) + public int mild_contention_httpResponse_servlet25() { + return threeResponses(servlet25); + } + + @Benchmark @Group("high_contention") @GroupThreads(8) + public int high_contention_httpResponse_servlet25() { + return threeResponses(servlet25); + } + + @Benchmark @Group("no_contention") @GroupThreads(1) + public int no_contention_status_reflection() throws Exception { + return threeStatusesReflection(); + } + + @Benchmark @Group("mild_contention") @GroupThreads(2) + public int mild_contention_status_reflection() throws Exception { + return threeStatusesReflection(); + } + + @Benchmark @Group("high_contention") @GroupThreads(8) + public int high_contention_status_reflection() throws Exception { + return threeStatusesReflection(); + } + + private int threeStatusesReflection() throws Exception { + // & to ensure null wasn't returned (forces NPE) + return ((int) Response1.class.getMethod("getStatus").invoke(new Response1())) & + ((int) Response2.class.getMethod("getStatus").invoke(new Response2())) & + ((int) Response3.class.getMethod("getStatus").invoke(new Response3())); + } + + // Convenience main entry-point + public static void main(String[] args) throws RunnerException { + Options opt = new OptionsBuilder() + .include(".*" + ServletRuntimeBenchmarks.class.getSimpleName() + ".*") + .build(); + + new Runner(opt).run(); + } + + class Response1 extends HttpServletResponseImpl { + } + + class Response2 extends HttpServletResponseImpl { + } + + class Response3 extends HttpServletResponseImpl { + } + + public static class HttpServletResponseImpl implements HttpServletResponse { + @Override public String getCharacterEncoding() { + return null; + } + + @Override public String getContentType() { + return null; + } + + @Override public ServletOutputStream getOutputStream() { + return null; + } + + @Override public PrintWriter getWriter() { + return null; + } + + @Override public void setCharacterEncoding(String charset) { + } + + @Override public void setContentLength(int len) { + } + + @Override public void setContentLengthLong(long len) { + + } + + @Override public void setContentType(String type) { + } + + @Override public void setBufferSize(int size) { + } + + @Override public int getBufferSize() { + return 0; + } + + @Override public void flushBuffer() { + } + + @Override public void resetBuffer() { + } + + @Override public boolean isCommitted() { + return false; + } + + @Override public void reset() { + } + + @Override public void setLocale(Locale loc) { + } + + @Override public Locale getLocale() { + return null; + } + + @Override public void addCookie(Cookie cookie) { + } + + @Override public boolean containsHeader(String name) { + return false; + } + + @Override public String encodeURL(String url) { + return null; + } + + @Override public String encodeRedirectURL(String url) { + return null; + } + + @Override public String encodeUrl(String url) { + return null; + } + + @Override public String encodeRedirectUrl(String url) { + return null; + } + + @Override public void sendError(int sc, String msg) { + } + + @Override public void sendError(int sc) { + } + + @Override public void sendRedirect(String location) { + } + + @Override public void setDateHeader(String name, long date) { + } + + @Override public void addDateHeader(String name, long date) { + } + + @Override public void setHeader(String name, String value) { + } + + @Override public void addHeader(String name, String value) { + } + + @Override public void setIntHeader(String name, int value) { + } + + @Override public void addIntHeader(String name, int value) { + } + + @Override public void setStatus(int sc) { + } + + @Override public void setStatus(int sc, String sm) { + } + + @Override public int getStatus() { + return 200; + } + + @Override public String getHeader(String name) { + return null; + } + + @Override public Collection getHeaders(String name) { + return null; + } + + @Override public Collection getHeaderNames() { + return null; + } + } +} diff --git a/instrumentation/servlet/src/it/servlet25/src/test/java/brave/servlet/ServletRuntime25Test.java b/instrumentation/servlet/src/it/servlet25/src/test/java/brave/servlet/ServletRuntime25Test.java new file mode 100644 index 0000000000..48b2eb54dc --- /dev/null +++ b/instrumentation/servlet/src/it/servlet25/src/test/java/brave/servlet/ServletRuntime25Test.java @@ -0,0 +1,154 @@ +package brave.servlet; + +import java.io.IOException; +import java.io.PrintWriter; +import java.lang.reflect.Field; +import java.util.Locale; +import javax.servlet.ServletOutputStream; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletResponse; +import org.eclipse.jetty.server.Response; +import org.junit.Before; +import org.junit.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +/** These tests must to be run in a classpath without servlet 3.x types */ +public class ServletRuntime25Test { + + @Test public void status_fromJetty() throws Exception { + Response jettyResponse = new Response(null); + Field field = Response.class.getDeclaredField("_status"); + field.setAccessible(true); + field.set(jettyResponse, 400); + assertThat(ServletRuntime.get().status(jettyResponse)) + .isEqualTo(400); + } + + @Test public void httpResponse_wrapsHttpServletResponse() throws Exception { + assertThat(ServletRuntime.get().httpResponse(new WithoutGetStatus())) + .isInstanceOf(ServletRuntime.Servlet25ServerResponseAdapter.class); + } + + @Test public void status_fromInvalidMethod() throws Exception { + assertThat(ServletRuntime.get().status(new WithInvalidGetStatus())) + .isNull(); + } + + public static class WithInvalidGetStatus extends WithoutGetStatus { + public String getStatus(){ + return "foo"; + } + } + + // intentionally won't compile if servlet > 2.5 is present! testing handling of missing getStatus + public static class WithoutGetStatus implements HttpServletResponse { + @Override public String getCharacterEncoding() { + return null; + } + + @Override public String getContentType() { + return null; + } + + @Override public ServletOutputStream getOutputStream() throws IOException { + return null; + } + + @Override public PrintWriter getWriter() throws IOException { + return null; + } + + @Override public void setCharacterEncoding(String charset) { + } + + @Override public void setContentLength(int len) { + } + + @Override public void setContentType(String type) { + } + + @Override public void setBufferSize(int size) { + } + + @Override public int getBufferSize() { + return 0; + } + + @Override public void flushBuffer() throws IOException { + } + + @Override public void resetBuffer() { + } + + @Override public boolean isCommitted() { + return false; + } + + @Override public void reset() { + } + + @Override public void setLocale(Locale loc) { + } + + @Override public Locale getLocale() { + return null; + } + + @Override public void addCookie(Cookie cookie) { + } + + @Override public boolean containsHeader(String name) { + return false; + } + + @Override public String encodeURL(String url) { + return null; + } + + @Override public String encodeRedirectURL(String url) { + return null; + } + + @Override public String encodeUrl(String url) { + return null; + } + + @Override public String encodeRedirectUrl(String url) { + return null; + } + + @Override public void sendError(int sc, String msg) throws IOException { + } + + @Override public void sendError(int sc) throws IOException { + } + + @Override public void sendRedirect(String location) throws IOException { + } + + @Override public void setDateHeader(String name, long date) { + } + + @Override public void addDateHeader(String name, long date) { + } + + @Override public void setHeader(String name, String value) { + } + + @Override public void addHeader(String name, String value) { + } + + @Override public void setIntHeader(String name, int value) { + } + + @Override public void addIntHeader(String name, int value) { + } + + @Override public void setStatus(int sc) { + } + + @Override public void setStatus(int sc, String sm) { + } + } +} diff --git a/instrumentation/servlet/src/main/java/brave/servlet/ServletRuntime.java b/instrumentation/servlet/src/main/java/brave/servlet/ServletRuntime.java index 4293a617da..1be349ac80 100644 --- a/instrumentation/servlet/src/main/java/brave/servlet/ServletRuntime.java +++ b/instrumentation/servlet/src/main/java/brave/servlet/ServletRuntime.java @@ -4,6 +4,10 @@ import brave.http.HttpServerHandler; import brave.internal.Nullable; import java.io.IOException; +import java.lang.reflect.Method; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; import javax.servlet.AsyncContext; import javax.servlet.AsyncEvent; import javax.servlet.AsyncListener; @@ -11,6 +15,7 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpServletResponseWrapper; +import zipkin2.Call; /** * Access to servlet version-specific features @@ -55,7 +60,7 @@ private static ServletRuntime findServletRuntime() { return new Servlet25(); } - private static final class Servlet3 extends ServletRuntime { + static final class Servlet3 extends ServletRuntime { @Override boolean isAsync(HttpServletRequest request) { return request.isAsyncStarted(); } @@ -112,7 +117,7 @@ static final class TracingAsyncListener implements AsyncListener { } } - private static final class Servlet25 extends ServletRuntime { + static final class Servlet25 extends ServletRuntime { @Override HttpServletResponse httpResponse(ServletResponse response) { return new Servlet25ServerResponseAdapter(response); } @@ -126,12 +131,57 @@ private static final class Servlet25 extends ServletRuntime { assert false : "this should never be called in Servlet 2.5"; } + // copy-on-write global reflection cache outperforms thread local copies + final AtomicReference, Object>> classToGetStatus = + new AtomicReference<>(new LinkedHashMap<>()); + static final String RETURN_NULL = "RETURN_NULL"; + + /** + * Eventhough the Servlet 2.5 version of HttpServletResponse doesn't have the getStatus method, + * routine servlet runtimes, do, for example {@code org.eclipse.jetty.server.Response} + */ @Override @Nullable Integer status(HttpServletResponse response) { if (response instanceof Servlet25ServerResponseAdapter) { // servlet 2.5 doesn't have get status return ((Servlet25ServerResponseAdapter) response).getStatusInServlet25(); } - return null; + Class clazz = response.getClass(); + Map, Object> classesToCheck = classToGetStatus.get(); + Object getStatusMethod = classesToCheck.get(clazz); + if (getStatusMethod == RETURN_NULL || + (getStatusMethod == null && classesToCheck.size() == 10)) { // limit size + return null; + } + + // Now, we either have a cached method or we have room to cache a method + if (getStatusMethod == null) { + if (clazz.isLocalClass() || clazz.isAnonymousClass()) return null; // don't cache + try { + // we don't check for accessibility as isAccessible is deprecated: just fail later + getStatusMethod = clazz.getMethod("getStatus"); + return (int) ((Method) getStatusMethod).invoke(response); + } catch (Throwable throwable) { + Call.propagateIfFatal(throwable); + getStatusMethod = RETURN_NULL; + return null; + } finally { + // regardless of success or fail, replace the cache + Map, Object> replacement = new LinkedHashMap<>(classesToCheck); + replacement.put(clazz, getStatusMethod); + classToGetStatus.set(replacement); // lost race will reset, but only up to size - 1 times + } + } + + // if we are here, we have a cached method, that "should" never fail, but we check anyway + try { + return (int) ((Method) getStatusMethod).invoke(response); + } catch (Throwable throwable) { + Call.propagateIfFatal(throwable); + Map, Object> replacement = new LinkedHashMap<>(classesToCheck); + replacement.put(clazz, RETURN_NULL); + classToGetStatus.set(replacement); // prefer overriding on failure + return null; + } } } diff --git a/instrumentation/servlet/src/test/java/brave/servlet/ServletRuntimeTest.java b/instrumentation/servlet/src/test/java/brave/servlet/ServletRuntimeTest.java new file mode 100644 index 0000000000..e236bd2e3f --- /dev/null +++ b/instrumentation/servlet/src/test/java/brave/servlet/ServletRuntimeTest.java @@ -0,0 +1,231 @@ +package brave.servlet; + +import java.io.PrintWriter; +import java.util.Collection; +import java.util.Locale; +import javax.servlet.ServletOutputStream; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletResponse; +import org.eclipse.jetty.server.Response; +import org.junit.Before; +import org.junit.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +public class ServletRuntimeTest { + ServletRuntime servlet25 = new ServletRuntime.Servlet25(); + + @Test public void servlet25_status_doesntParseAnonymousTypes() throws Exception { + // while looks nice, this will overflow our cache + Response jettyResponse = new Response(null) { + @Override public int getStatus() { + throw new AssertionError(); + } + }; + assertThat(servlet25.status(jettyResponse)) + .isNull(); + } + + @Test public void servlet25_status_doesntParseLocalTypes() throws Exception { + // while looks nice, this will overflow our cache + class LocalResponse extends HttpServletResponseImpl { + } + assertThat(servlet25.status(new LocalResponse())) + .isNull(); + } + + class ExceptionResponse extends HttpServletResponseImpl { + @Override public int getStatus() { + throw new IllegalStateException("foo"); + } + } + + @Test public void servlet25_status_nullOnException() throws Exception { + assertThat(servlet25.status(new ExceptionResponse())) + .isNull(); + } + + class Response1 extends HttpServletResponseImpl { + } + + class Response2 extends HttpServletResponseImpl { + } + + class Response3 extends HttpServletResponseImpl { + } + + class Response4 extends HttpServletResponseImpl { + } + + class Response5 extends HttpServletResponseImpl { + } + + class Response6 extends HttpServletResponseImpl { + } + + class Response7 extends HttpServletResponseImpl { + } + + class Response8 extends HttpServletResponseImpl { + } + + class Response9 extends HttpServletResponseImpl { + } + + class Response10 extends HttpServletResponseImpl { + } + + class Response11 extends HttpServletResponseImpl { + } + + @Test public void servlet25_status_cachesUpToTenTypes() throws Exception { + assertThat(servlet25.status(new Response1())) + .isEqualTo(200); + assertThat(servlet25.status(new Response2())) + .isEqualTo(200); + assertThat(servlet25.status(new Response3())) + .isEqualTo(200); + assertThat(servlet25.status(new Response4())) + .isEqualTo(200); + assertThat(servlet25.status(new Response5())) + .isEqualTo(200); + assertThat(servlet25.status(new Response6())) + .isEqualTo(200); + assertThat(servlet25.status(new Response7())) + .isEqualTo(200); + assertThat(servlet25.status(new Response8())) + .isEqualTo(200); + assertThat(servlet25.status(new Response9())) + .isEqualTo(200); + assertThat(servlet25.status(new Response10())) + .isEqualTo(200); + assertThat(servlet25.status(new Response11())) + .isNull(); + } + + public static class HttpServletResponseImpl implements HttpServletResponse { + @Override public String getCharacterEncoding() { + return null; + } + + @Override public String getContentType() { + return null; + } + + @Override public ServletOutputStream getOutputStream() { + return null; + } + + @Override public PrintWriter getWriter() { + return null; + } + + @Override public void setCharacterEncoding(String charset) { + } + + @Override public void setContentLength(int len) { + } + + @Override public void setContentType(String type) { + } + + @Override public void setBufferSize(int size) { + } + + @Override public int getBufferSize() { + return 0; + } + + @Override public void flushBuffer() { + } + + @Override public void resetBuffer() { + } + + @Override public boolean isCommitted() { + return false; + } + + @Override public void reset() { + } + + @Override public void setLocale(Locale loc) { + } + + @Override public Locale getLocale() { + return null; + } + + @Override public void addCookie(Cookie cookie) { + } + + @Override public boolean containsHeader(String name) { + return false; + } + + @Override public String encodeURL(String url) { + return null; + } + + @Override public String encodeRedirectURL(String url) { + return null; + } + + @Override public String encodeUrl(String url) { + return null; + } + + @Override public String encodeRedirectUrl(String url) { + return null; + } + + @Override public void sendError(int sc, String msg) { + } + + @Override public void sendError(int sc) { + } + + @Override public void sendRedirect(String location) { + } + + @Override public void setDateHeader(String name, long date) { + } + + @Override public void addDateHeader(String name, long date) { + } + + @Override public void setHeader(String name, String value) { + } + + @Override public void addHeader(String name, String value) { + } + + @Override public void setIntHeader(String name, int value) { + } + + @Override public void addIntHeader(String name, int value) { + } + + @Override public void setStatus(int sc) { + } + + @Override public void setStatus(int sc, String sm) { + } + + @Override public int getStatus() { + return 200; + } + + @Override public String getHeader(String name) { + return null; + } + + @Override public Collection getHeaders(String name) { + return null; + } + + @Override public Collection getHeaderNames() { + return null; + } + } +}