ptrd / kwik

A QUIC client, client library and server implementation in Java. Supports HTTP3 with "Flupke" add-on.
GNU General Public License v3.0
396 stars 55 forks source link

Project Loom / Virtual Threads support #53

Open Mechite opened 2 months ago

Mechite commented 2 months ago

How does Kwik play with virtual threads?

I see many usages of synchronized keyword in current implementation methods (easy reference is Kwik log impl), perhaps ReentrantLock is extremely easy to completely migrate to, to prevent pinning. Use of synchronized is hardly an issue at all, if the pinning does not occur frequently, but we can squeeze out some more performance when virtual threads are used, with supported synchronization technique.

I have a feeling that Kwik would already play very well and see major performance gain with virtual threads, due to its nature being written entirely in 100% Java with no dependency on JNI as far as it appears.

If we can support choosing the use of it, for underlying concurrency systems, with a configuration option, that would be ideal, if Kwik currently builds with a version of Java that supports this feature, or we can provide support with it potentially e.g. accepting an ExecutorService, ThreadFactory etc with builders/arguments, etc.

For official guidance on adopting virtual threads - https://docs.oracle.com/en/java/javase/21/core/virtual-threads.html - "Virtual Threads: An Adoption Guide" section. One can also refer to the case studies from those who have good support, in similar libraries.

There may be more optimizations we can make. If it is of motive to optimize Kwik for this, this issue could serve to track these.

ptrd commented 2 months ago

Support for virtual threads has often crossed my mind. I have also experimented a few times with replacing synchronized methods by ReentrantLock's, but as I didn't see (measure) any direct performance gain, these commits didn't make it to master yet. That the Sysout logger has a negative impact on performance is certainly true; in performance tests I usually use the NullLogger instead. So, I'm certainly supporting this idea. I'll create a branch for it and see if i my earlier re-entrant-lock experiments are good enough to integrate. And you'll have a branch for pull requests for this topic.

Mechite commented 2 months ago

A nice JMH benchmark might be a nice module to try and implement into this repo.

I didn't see (measure) any direct performance gain

I think creating a benchmark of Kwik against other options, e.g. TCP unicast with the multitude of very optimized options out there, Aeron, etc, would provide a very nice base ground.

Profiling this benchmark can easily show us, where we could implement more structured concurrency vs locking, and also simply give a brief view of where Kwik stands in terms of performance.

corite commented 1 week ago

FYI: starting with OpenJDK version 24, a VirtualThread will no longer be pinned whilst holding a monitor. Therefore the above mentioned performance concerns no longer apply, because monitor locks are (if VirtualThread pinning is out of the picture) not intrinsically slower or faster than ConcurrentLock implementations.

Of course, this will only apply to Java 24 and later. However, due to deadlocking problems that VirtualThread pinning causes (for an example with monitors have a look at the Netflix blog), IMHO it is very questionable to use VirtualThreads before JDK24 except if you can tolerate your JVM spontaneously deadlocking sometimes (btw, this can even happen if you do everything correctly with ConcurrentLock and nothing pins the VirtualThread).

ptrd commented 1 week ago

Hi @corite , thanks for your comment, very helpful. The remark on locks not being intrinsically slower than Locks made me realise I should not dogmatically replace "synchronized" by locks.