vert.x
vert.x copied to clipboard
Fix InboundBuffer::pause & write performance
This patch improve Quarkus techempower plaintext throughput of ~5% in our lab; nothing fancy but seems related to the excessive synchronization which, by using instance field to synchronize with, doesn't allow the JVM to perform the https://shipilev.net/jvm/anatomy-quarks/1-lock-coarsening-for-loops/ (lock coarsening) optimization.
I've applied an optimization to onEnd
which benefit vertx as well, which often observe non-paused requests
i.e. reducing the synchronization over the connection while completing the request.
@vietj This change here is not in conflict with https://github.com/eclipse-vertx/vert.x/pull/5143 given that it works on different synchronization(s) happening in a inefficient way, so, if you agree on the changes, could be safely merged.
Just for reference, it improves by ~5% few benchmarks using plaintext, so, not a game changer; but given that fixing the presence of pausing in Quarkus is NOT possible nor is possible to improve the inbound buffer to not create any array q, in case we end a request while still paused, this is the less destructive and simpler change to improve things I could think about.
Said that, sadly profilers can consistently lie and, in an experiment, I've tried removing InboundBuffer::write
code just to check if it improves performance, and it seems not (!); instead its caller io/vertx/core/http/impl/Http1xServerConnection.handleMessage
has been blame, which will be improved by https://github.com/eclipse-vertx/vert.x/pull/5143
@vietj ping
@vietj how this look bud?
this looks good, I need to review it.
can you have a quick look at https://github.com/eclipse-vertx/vert.x/pull/5164/files ?
@franz1981 before proceeding with this, I think we can review and merge https://github.com/eclipse-vertx/vert.x/pull/5143/files which does remove synchronization use in Http1xServerConnection
@vietj I've addressed your comments and added some tests for the additional unsynchronized methods.
@vietj this is fine than? So we can bring it in the next quarkus release
Ping @vietj ? This is ok to merge? So I can verify on the next techempower round if helps?