Fix some instance check type pollution (#10893)

Measured using https://github.com/RedHatPerf/type-pollution-agent in the techempower benchmarks. This patch fixes some of the instanceof type pollution that could cause scalability issues with many CPUs, and also some instanceof misses that are less problematic but still worth avoiding.
micronaut-projects · Jun 7, 2024 · f9551ce · f9551ce
1 parent f444b3f
commit f9551ce
Show file tree

Hide file tree

Showing 8 changed files with 55 additions and 17 deletions.
diff --git a/core-reactive/src/main/java/io/micronaut/core/async/publisher/Publishers.java b/core-reactive/src/main/java/io/micronaut/core/async/publisher/Publishers.java
@@ -424,12 +424,16 @@ protected void doOnComplete() {
      * @return True if it is
      */
     public static boolean isConvertibleToPublisher(Class<?> type) {
+        // first, check for some common types without instanceof
+        if (type == Publisher.class) {
+            return true;
+        }
+        if (type.isPrimitive() || type.getName().startsWith("java.") || type.isArray()) {
+            return false;
+        }
         if (Publisher.class.isAssignableFrom(type)) {
             return true;
         } else {
-            if (type.isPrimitive() || type.getName().startsWith("java.")) {
-                return false;
-            }
             for (Class<?> reactiveType : REACTIVE_TYPES) {
                 if (reactiveType.isAssignableFrom(type)) {
                     return true;
@@ -454,7 +458,10 @@ private static String packageOf(Class<?> type) {
      * @return True if it is
      */
     public static boolean isConvertibleToPublisher(Object object) {
-        if (object == null) {
+        if (object == null ||
+            // check some common types for performance
+            object instanceof String || object instanceof byte[]) {
+
             return false;
         }
         if (object instanceof Publisher) {

diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java
@@ -20,6 +20,7 @@
 import io.micronaut.core.annotation.NonNull;
 import io.micronaut.core.annotation.Nullable;
 import io.micronaut.core.async.publisher.Publishers;
+import io.micronaut.core.attr.AttributeHolder;
 import io.micronaut.core.bind.ArgumentBinder;
 import io.micronaut.core.convert.ArgumentConversionContext;
 import io.micronaut.core.convert.ConversionService;
@@ -62,6 +63,7 @@
 import io.micronaut.http.server.netty.body.NettyByteBody;
 import io.micronaut.http.server.netty.handler.Http2ServerHandler;
 import io.micronaut.http.server.netty.multipart.NettyCompletedFileUpload;
+import io.micronaut.web.router.DefaultUriRouteMatch;
 import io.micronaut.web.router.RouteMatch;
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
@@ -384,7 +386,9 @@ public <T1> Optional<T1> getBody(ArgumentConversionContext<T1> conversionContext
      */
     @Internal
     public void release() {
-        RouteMatch<?> routeMatch = (RouteMatch<?>) getAttribute(HttpAttributes.ROUTE_MATCH).orElse(null);
+        Object routeMatchO = ((AttributeHolder) this).getAttribute(HttpAttributes.ROUTE_MATCH).orElse(null);
+        // usually this is a DefaultUriRouteMatch, avoid scalability issues here
+        RouteMatch<?> routeMatch = routeMatchO instanceof DefaultUriRouteMatch<?, ?> urm ? urm : (RouteMatch<?>) routeMatchO;
         if (routeMatch != null) {
             // discard parameters that have already been bound
             for (Object toDiscard : routeMatch.getVariableValues().values()) {
@@ -404,7 +408,14 @@ public void release() {
             formRouteCompleter.release();
         }
         if (attributes != null) {
-            attributes.values().forEach(this::releaseIfNecessary);
+            attributes.forEach((k, v) -> {
+                //noinspection StringEquality
+                if (k == HttpAttributes.ROUTE_MATCH.toString() || k == HttpAttributes.ROUTE_INFO.toString() || v instanceof String) {
+                    // perf: avoid an instanceof in releaseIfNecessary
+                    return;
+                }
+                releaseIfNecessary(v);
+            });
         }
     }
 

diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyRequestLifecycle.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyRequestLifecycle.java
@@ -25,6 +25,7 @@
 import io.micronaut.http.HttpStatus;
 import io.micronaut.http.annotation.Body;
 import io.micronaut.http.body.ByteBody;
+import io.micronaut.http.netty.NettyMutableHttpResponse;
 import io.micronaut.http.server.RequestLifecycle;
 import io.micronaut.http.server.netty.body.NettyByteBody;
 import io.micronaut.http.server.netty.body.StreamingMultiObjectBody;
@@ -92,7 +93,10 @@ void handleNormal(NettyHttpRequest<?> request) {
             }
             ImperativeExecutionFlow<HttpResponse<?>> imperativeFlow = result.tryComplete();
             if (imperativeFlow != null) {
-                rib.writeResponse(outboundAccess, request, imperativeFlow.getValue(), imperativeFlow.getError());
+                Object value = ((ImperativeExecutionFlow<?>) imperativeFlow).getValue();
+                // usually this is a MutableHttpResponse, avoid scalability issues here
+                HttpResponse<?> response = value instanceof NettyMutableHttpResponse<?> mut ? mut : (HttpResponse<?>) value;
+                rib.writeResponse(outboundAccess, request, response, imperativeFlow.getError());
             } else {
                 result.onComplete((response, throwable) -> rib.writeResponse(outboundAccess, request, response, throwable));
             }

diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/RoutingInBoundHandler.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/RoutingInBoundHandler.java
@@ -58,6 +58,7 @@
 import io.micronaut.http.server.netty.configuration.NettyHttpServerConfiguration;
 import io.micronaut.http.server.netty.handler.OutboundAccess;
 import io.micronaut.http.server.netty.handler.RequestHandler;
+import io.micronaut.web.router.DefaultUrlRouteInfo;
 import io.micronaut.web.router.RouteInfo;
 import io.micronaut.web.router.resource.StaticResourceResolver;
 import io.netty.buffer.ByteBuf;
@@ -299,7 +300,9 @@ private void encodeHttpResponse(
         Object body) {
         MutableHttpResponse<?> response = httpResponse.toMutableResponse();
         if (nettyRequest.getMethod() != HttpMethod.HEAD && body != null) {
-            @SuppressWarnings("unchecked") final RouteInfo<Object> routeInfo = response.getAttribute(HttpAttributes.ROUTE_INFO, RouteInfo.class).orElse(null);
+            Object routeInfoO = response.getAttribute(HttpAttributes.ROUTE_INFO).orElse(null);
+            // usually this is a UriRouteInfo, avoid scalability issues here
+            @SuppressWarnings("unchecked") final RouteInfo<Object> routeInfo = (RouteInfo<Object>) (routeInfoO instanceof DefaultUrlRouteInfo<?, ?> uri ? uri : (RouteInfo<?>) routeInfoO);
 
             if (Publishers.isConvertibleToPublisher(body)) {
                 response.body(null);
@@ -320,7 +323,9 @@ private void encodeHttpResponse(
                 responseBodyType = Argument.of((Class<Object>) body.getClass());
             }
             if (responseMediaType == null) {
-                if (body instanceof MediaTypeProvider mediaTypeProvider) {
+                // perf: check for common body types
+                //noinspection ConditionCoveredByFurtherCondition
+                if (!(body instanceof String) && !(body instanceof byte[]) && body instanceof MediaTypeProvider mediaTypeProvider) {
                     responseMediaType = mediaTypeProvider.getMediaType();
                 } else if (routeInfo != null) {
                     responseMediaType = routeExecutor.resolveDefaultResponseContentType(nettyRequest, routeInfo);

diff --git a/...r-netty/src/main/java/io/micronaut/http/server/netty/handler/PipeliningServerHandler.java b/...r-netty/src/main/java/io/micronaut/http/server/netty/handler/PipeliningServerHandler.java
@@ -43,6 +43,7 @@
 import io.netty.handler.codec.compression.ZlibWrapper;
 import io.netty.handler.codec.http.DefaultFullHttpResponse;
 import io.netty.handler.codec.http.DefaultHttpContent;
+import io.netty.handler.codec.http.DefaultHttpRequest;
 import io.netty.handler.codec.http.DefaultHttpResponse;
 import io.netty.handler.codec.http.DefaultLastHttpContent;
 import io.netty.handler.codec.http.FullHttpRequest;
@@ -398,12 +399,13 @@ void read(Object message) {
                 headers.add(HttpHeaderNames.TRANSFER_ENCODING, HttpHeaderValues.CHUNKED);
             }
 
-            boolean full = request instanceof FullHttpRequest;
+            // getClass for performance
+            boolean full = request.getClass() != DefaultHttpRequest.class && request instanceof FullHttpRequest;
             if (full && decompressionChannel == null) {
                 requestHandler.accept(ctx, request, createImmediateByteBody(ctx.channel().eventLoop(), bodySizeLimits, ((FullHttpRequest) request).content()), outboundAccess);
             } else if (!hasBody(request)) {
                 inboundHandler = droppingInboundHandler;
-                if (message instanceof HttpContent) {
+                if (full) {
                     inboundHandler.read(message);
                 }
                 if (decompressionChannel != null) {
@@ -709,9 +711,11 @@ void handleUpstreamError(Throwable cause) {
     private final class DroppingInboundHandler extends InboundHandler {
         @Override
         void read(Object message) {
-            ((HttpContent) message).release();
-            if (message instanceof LastHttpContent) {
+            if (message instanceof LastHttpContent lhc) {
+                lhc.release();
                 inboundHandler = baseInboundHandler;
+            } else {
+                ((HttpContent) message).release();
             }
         }
 

diff --git a/http-server/src/main/java/io/micronaut/http/server/RequestLifecycle.java b/http-server/src/main/java/io/micronaut/http/server/RequestLifecycle.java
@@ -41,6 +41,7 @@
 import io.micronaut.inject.qualifiers.Qualifiers;
 import io.micronaut.json.JsonSyntaxException;
 import io.micronaut.web.router.DefaultRouteInfo;
+import io.micronaut.web.router.DefaultUriRouteMatch;
 import io.micronaut.web.router.RouteInfo;
 import io.micronaut.web.router.RouteMatch;
 import io.micronaut.web.router.UriRouteMatch;
@@ -196,7 +197,9 @@ private ExecutionFlow<HttpResponse<?>> executeRoute(HttpRequest<?> request,
     private ExecutionFlow<HttpResponse<?>> callRoute(ExecutionFlow<RouteMatch<?>> flux,
                                                      HttpRequest<?> filteredRequest,
                                                      PropagatedContext propagatedContext) {
-        RouteMatch<?> routeMatch = flux.tryCompleteValue();
+        Object o = ((ExecutionFlow<?>) flux).tryCompleteValue();
+        // usually this is a DefaultUriRouteMatch, avoid scalability issues here
+        RouteMatch<?> routeMatch = o instanceof DefaultUriRouteMatch<?, ?> urm ? urm : (RouteMatch<?>) o;
         if (routeMatch != null) {
             return routeExecutor.callRoute(propagatedContext, routeMatch, filteredRequest);
         }
@@ -207,7 +210,9 @@ private ExecutionFlow<HttpResponse<?>> handleStatusException(ExecutionFlow<HttpR
                                                                  HttpRequest<?> request,
                                                                  RouteMatch<?> routeMatch,
                                                                  PropagatedContext propagatedContext) {
-        HttpResponse<?> response = flux.tryCompleteValue();
+        Object o = ((ExecutionFlow<?>) flux).tryCompleteValue();
+        // usually this is a MutableHttpResponse, avoid scalability issues here
+        HttpResponse<?> response = o instanceof MutableHttpResponse<?> mut ? mut : (HttpResponse<?>) o;
         if (response != null) {
             return handleStatusException(request, response, routeMatch, propagatedContext);
         }

diff --git a/http-server/src/main/java/io/micronaut/http/server/RouteExecutor.java b/http-server/src/main/java/io/micronaut/http/server/RouteExecutor.java
@@ -434,7 +434,9 @@ private boolean isSingle(RouteInfo<?> finalRoute, Class<?> bodyClass) {
     }
 
     private ExecutionFlow<MutableHttpResponse<?>> fromImperativeExecute(PropagatedContext propagatedContext, HttpRequest<?> request, RouteInfo<?> routeInfo, Object body) {
-        if (body instanceof HttpResponse<?> httpResponse) {
+        // performance optimization: check for common body types
+        //noinspection ConditionCoveredByFurtherCondition
+        if (!(body instanceof String) && !(body instanceof byte[]) && body instanceof HttpResponse<?> httpResponse) {
             MutableHttpResponse<?> outgoingResponse = httpResponse.toMutableResponse();
             final Argument<?> bodyArgument = routeInfo.getReturnType().getFirstTypeVariable().orElse(Argument.OBJECT_ARGUMENT);
             if (bodyArgument.isAsyncOrReactive()) {

diff --git a/router/src/main/java/io/micronaut/web/router/DefaultUriRouteMatch.java b/router/src/main/java/io/micronaut/web/router/DefaultUriRouteMatch.java
@@ -37,7 +37,7 @@
  * @since 1.0
  */
 @Internal
-class DefaultUriRouteMatch<T, R> extends AbstractRouteMatch<T, R> implements UriRouteMatch<T, R> {
+public final class DefaultUriRouteMatch<T, R> extends AbstractRouteMatch<T, R> implements UriRouteMatch<T, R> {
 
     private final UriMatchInfo matchInfo;
     private final UriRouteInfo<T, R> uriRouteInfo;