RESTEasy http performance #5220
Conversation
There was a problem hiding this comment.
Looks good to me but I would like @stuartwdouglas to check the isFlushed() method shouldn't have been used somehow.
| } | ||
|
|
||
| @Override | ||
| public ResteasyAsynchronousContext getAsyncContext() { | ||
| return executionContext; | ||
| } | ||
|
|
||
| public boolean isFlushed() { | ||
| return flushed; | ||
| } |
There was a problem hiding this comment.
@stuartwdouglas just to be sure that this shouldn't have been consumed somehow?
|
Test failures doesn't look related, I asked for a recheck. |
|
Is the need for @stuartwdouglas 's opinion blocking? I'd suggest it shouldn't, as while it indeed looks fishy, that was definitely dead code. I can open a different issue to have him have a look as independent task. Or if you'd rather wait for his feedback first I could split this PR so to get the performance fix already :) |
|
For more context: making that address field lazy makes a significant difference ;) |
|
LGTM |
|
Is there a reason why I can't rerun the failed tests? I used to be able to :) |
|
It comes and go. Sometimes I don't have the buttons either. |
|
Gotcha, thanks! |
| 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 = () -> { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Maybe don't bother then, it is probably not a big deal.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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 😁
There was a problem hiding this comment.
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!
A couple minor optimisations in VertxHttpRequest based on profiling data.
Also, removed some unused fields.