vert.x icon indicating copy to clipboard operation
vert.x copied to clipboard

Fix InboundBuffer::pause & write performance

Open franz1981 opened this issue 11 months ago • 6 comments

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.

franz1981 avatar Mar 06 '24 15:03 franz1981

@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

franz1981 avatar Mar 13 '24 13:03 franz1981

@vietj ping

franz1981 avatar Mar 18 '24 10:03 franz1981

@vietj how this look bud?

franz1981 avatar Mar 27 '24 09:03 franz1981

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 ?

vietj avatar Mar 27 '24 09:03 vietj

@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 avatar Mar 27 '24 14:03 vietj

@vietj I've addressed your comments and added some tests for the additional unsynchronized methods.

franz1981 avatar Apr 11 '24 16:04 franz1981

@vietj this is fine than? So we can bring it in the next quarkus release

franz1981 avatar May 06 '24 07:05 franz1981

Ping @vietj ? This is ok to merge? So I can verify on the next techempower round if helps?

franz1981 avatar May 16 '24 06:05 franz1981