Save recursive locks on websocket writeFrame

[franz1981]

Vertx is using a nested series of synchronized over the connection field which costs keep on piling-up; this cost is particularly evident while executing writeFrame outside of I/O threads.

https://github.com/franz1981/java-puzzles/blob/583d468a58a6ecaa5e7c7c300895392638f688dd/src/main/java/red/hat/puzzles/concurrent/LockCoarsening.java#L76-L85

contains some microbenchmarks results which show how bad the performance is even in the uncontended case, and it can just become worse under contention.

This is addressing quarkusio/quarkus#39148 (comment)
by removing the nested serie of synchronized over connection, reducing them to the bare minimum i.e. just 2 in the best case scenario

[vietj]

are we trying to optimize WebSocket handhsake here ? or fix bug ?

[franz1981]

none of the two; or better, as shown in the jmh benchmarking linked in the description, the vertx pattern to acquire synchronized locks on a field, doesn't really work as expected, after biased locking is gone - and it could be improved by not having nested synchronized blocks

[vietj]

maybe that is a smell and we should not have a reject method for a WebSocket, i.e any WebSocket processed should be already accepted

[vietj]

still not clear what it changes , can you clearly describe what is changed and what is the motivation (like in netty) without referring to a quarkus comment ?

[mkouba]

So my understanding of the new ServerWebSocketImpl#tryHandshake() impl is that it attempts to read the volatile field first and if not null then no synchronized block is needed. This is a common pattern and it should not break anything ;-).

I'm not so sure about the AtomicReferenceFieldUpdater#lazySet() used to update the status. @franz1981 what's the diff compared to AtomicReferenceFieldUpdater#set()?

WebSocketImplBase#unsafeWriteFrame() and WebSocketImplBase#unsafeIsClosed() are meant to avoid one unnecessary nested synchronized block, right?

maybe that is a smell and we should not have a reject method for a WebSocket, i.e any WebSocket processed should be already accepted

Well, maybe but you can't just remove ServerWebSocket#reject() in 4.x, right?

and what is the motivation (like in netty) without referring to a quarkus comment ?

I believe that the motivation is to speedup writing messages to a server WebSocket from a non-event loop thread (e.g. from a worker thread) because as stated in the javadoc of io.vertx.core.http.impl.ServerWebSocketImpl: "This class is optimised for performance when used on the same event loop. However it can be used safely from other threads.".

[franz1981]

This is a common pattern and it should not break anything

Yep; for the same purpose of the unsafe methods: to save nested synchronized calls - which are costly (!).

I'm not so sure about the AtomicReferenceFieldUpdater#lazySet() used to update the status. @franz1981 what's the diff compared to AtomicReferenceFieldUpdater#set()?

Here: https://psy-lob-saw.blogspot.com/2012/12/atomiclazyset-is-performance-win-for.html
This is still valuable right now as well: on JMM 9 it has been replaced with a more meaningful name i.e. VarHandle::setRelease ; and it still have happens-before guarantees, just with a cheaper cost (and loosing sequential consistency, which we don't care here).

WebSocketImplBase#unsafeWriteFrame() and WebSocketImplBase#unsafeIsClosed() are meant to avoid one unnecessary nested synchronized block, right?

Yep, that's the main purpose: the assert there are indeed to shield developers from misuses and make clear the expected invariants: I usually name those unguardedSomething but is not a much better name TBH.

@vietj PTAL