Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RESTEasy http performance #5220

Merged
merged 4 commits into from
Nov 7, 2019
Merged

RESTEasy http performance #5220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
29a9737
Make VertxHttpRequest a final class
Sanne Nov 5, 2019
c69e180
Performance: VertxHttpRequest should allocate attributes map lazily
Sanne Nov 5, 2019
d8c4467
Performance: try resolve the remote Host address lazily
Sanne Nov 5, 2019
f1242b6
Remove some dead code
Sanne Nov 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 38 additions & 41 deletions ...steasy/runtime/src/main/java/io/quarkus/resteasy/runtime/standalone/VertxHttpRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@

import java.io.IOException;
import java.io.InputStream;
import java.util.Collections;
import java.util.Date;
import java.util.Enumeration;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
import java.util.concurrent.TimeUnit;
import java.util.function.Supplier;

import javax.ws.rs.ServiceUnavailableException;
import javax.ws.rs.container.AsyncResponse;
Expand Down Expand Up @@ -37,32 +39,26 @@
* @author Kristoffer Sjogren
* @version $Revision: 1 $
*/
public class VertxHttpRequest extends BaseHttpRequest {
protected ResteasyHttpHeaders httpHeaders;
protected SynchronousDispatcher dispatcher;
protected String httpMethod;
protected String remoteHost;
protected InputStream inputStream;
protected Map<String, Object> attributes = new HashMap<String, Object>();
protected VertxHttpResponse response;
private final boolean is100ContinueExpected;
public final class VertxHttpRequest extends BaseHttpRequest {
private ResteasyHttpHeaders httpHeaders;
private String httpMethod;
private Supplier<String> remoteHost;
private InputStream inputStream;
private Map<String, Object> attributes;
private VertxHttpResponse response;
private VertxExecutionContext executionContext;
private final Context context;
private volatile boolean flushed;

public VertxHttpRequest(Context context,
ResteasyHttpHeaders httpHeaders,
ResteasyUriInfo uri,
String httpMethod,
String remoteHost,
Supplier<String> remoteHost,
SynchronousDispatcher dispatcher,
VertxHttpResponse response,
boolean is100ContinueExpected) {
VertxHttpResponse response) {
super(uri);
this.context = context;
this.is100ContinueExpected = is100ContinueExpected;
this.response = response;
this.dispatcher = dispatcher;
this.httpHeaders = httpHeaders;
this.httpMethod = httpMethod;
this.remoteHost = remoteHost;
Expand All @@ -81,44 +77,50 @@ public void setHttpMethod(String method) {

@Override
public Enumeration<String> getAttributeNames() {
Enumeration<String> en = new Enumeration<String>() {
private Iterator<String> it = attributes.keySet().iterator();

@Override
public boolean hasMoreElements() {
return it.hasNext();
}
final Map<String, Object> attributes = this.attributes;
if (attributes == null) {
return Collections.emptyEnumeration();
} else {
Enumeration<String> en = new Enumeration<String>() {
private Iterator<String> it = attributes.keySet().iterator();

@Override
public boolean hasMoreElements() {
return it.hasNext();
}

@Override
public String nextElement() {
return it.next();
}
};
return en;
@Override
public String nextElement() {
return it.next();
}
};
return en;
}
}

@Override
public ResteasyAsynchronousContext getAsyncContext() {
return executionContext;
}

public boolean isFlushed() {
return flushed;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stuartwdouglas just to be sure that this shouldn't have been consumed somehow?


@Override
public Object getAttribute(String attribute) {
return attributes.get(attribute);
return attributes != null ? attributes.get(attribute) : null;
}

@Override
public void setAttribute(String name, Object value) {
if (attributes == null) {
attributes = new HashMap<String, Object>();
}
attributes.put(name, value);
}

@Override
public void removeAttribute(String name) {
attributes.remove(name);
if (attributes != null) {
attributes.remove(name);
}
}

@Override
Expand All @@ -128,12 +130,12 @@ public HttpHeaders getHttpHeaders() {

@Override
public String getRemoteHost() {
return remoteHost;
return remoteHost.get();
}

@Override
public String getRemoteAddress() {
return remoteHost;
return remoteHost.get();
}

@Override
Expand All @@ -155,10 +157,6 @@ public VertxHttpResponse getResponse() {
return response;
}

public boolean is100ContinueExpected() {
return is100ContinueExpected;
}

@Override
public void forward(String path) {
throw new NotImplementedYetException();
Expand Down Expand Up @@ -309,7 +307,6 @@ public boolean cancel(int retryAfter) {
}

protected synchronized void vertxFlush() {
flushed = true;
try {
vertxResponse.finish();
} catch (IOException e) {
Expand Down
15 changes: 10 additions & 5 deletions ...asy/runtime/src/main/java/io/quarkus/resteasy/runtime/standalone/VertxRequestHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import java.io.IOException;
import java.io.InputStream;
import java.util.function.Supplier;

import javax.enterprise.inject.Instance;
import javax.enterprise.inject.spi.CDI;
Expand Down Expand Up @@ -96,12 +97,16 @@ private void dispatch(RoutingContext routingContext, InputStream is, VertxOutput
HttpServerResponse response = request.response();
VertxHttpResponse vertxResponse = new VertxHttpResponse(request, dispatcher.getProviderFactory(),
request.method(), allocator, output);
// client address may not be available with VirtualHttp
SocketAddress socketAddress = request.remoteAddress();
String host = socketAddress != null ? socketAddress.host() : null;
// client address may not be available with VirtualHttp;
// using a supplier to make the remote Address resolution lazy: often it's not needed.
Supplier<String> hostNameProvider = () -> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We try and avoid lambdas in the runtime part of the code as they are much more costly in terms of metaspsace than an anonymous class.

If this is expensive shouldn't the result of this be cached either in the Supplier or in VertxHttpRequest in case getRemoteAddress is called multiple times?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I can change the lambda to an anonymous class. I know the metaspace cost, TBH I gave up on avoiding them in runtime code as we have many of them anyway, I had the impression nobody except me was still following that recommendation :)

It's not clear to me if it's worth caching the result. I can do that "just in case", but FYI the getter is not invoked during hot-path code in the benchmarks I've run: I verified this by initially running a modified version which was throing an exception.
I had to remove the exception-throwing code as some tests would fail, but it seems seldomly used - I don't know for sure though I guess I could have seen an happy path in my runs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P.S. the reason I ask if it's worth caching, is because null is a valid value - so to cache it I will need to add multiple additional fields.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe don't bother then, it is probably not a big deal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sanne Just for the record, you're not the only one avoiding lambdas in runtime code. We have some in build time but we really try to be careful for the runtime part.

But certainly some have slipped along the way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm what @gsmet mentions from my part as well.
We have instructed contributors multiple times to avoid them.
Of course some can sneak in 😁

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean there's plenty in the libraries we use - even if we avoid them in io.quarkus.* code ...

But I agree with the principle of not opting for a lambda without a strong reason, I'll be more careful. Thanks!

SocketAddress socketAddress = request.remoteAddress();
String host = socketAddress != null ? socketAddress.host() : null;
return host;
};

VertxHttpRequest vertxRequest = new VertxHttpRequest(ctx, headers, uriInfo, request.rawMethod(),
host, dispatcher.getDispatcher(), vertxResponse, false);
VertxHttpRequest vertxRequest = new VertxHttpRequest(ctx, headers, uriInfo, request.rawMethod(), hostNameProvider,
dispatcher.getDispatcher(), vertxResponse);
vertxRequest.setInputStream(is);
try {
ResteasyContext.pushContext(SecurityContext.class, new QuarkusResteasySecurityContext(request));
Expand Down