Fazecast/jSerialComm

Support for virtual threads

xberkhout opened this issue · 11 comments

Java 21 (coming after the summer) will have official support for Virtual Threads. The advantage is that thread creation and blocking is cheaper, but that only works right with external locking (ReentrantLock) and not with internal locking (synchronized). My question is if your code is prepared already for virtual threads with respect to locking?

A virtual thread is pinned to the carrier thread in case of JNI calls, so the thread synchronization solution doesn't matter much in case of this library.
Besides, I think it is very rare that thread count is a practical problem in case of serial connections, so virtual thread support is not so important IMHO.

I know a virtual thread is pinned to a carrier thread in case of JNI calls, but if the blocking is in the java code it does matter. If the blocking is in the C-code it doesn't.

Thread count may not be a problem, but memory usage can be an issue. I use many communication channels and if I don't know upfront if it will be a serial or network connection and I choose a virtual thread then it should be treated as good as possible. I am only asking if you could consider replacing a synchronized block with a ReentrantLock in your Java code if that applies to your code, so I don't have thread pinning while waiting on a Java monitor.

I searched for synchronized in the project (as you you have probably done too before asking), synchronized is used mainly in

  • listing ports,
  • opening/closing ports, and
  • port configuration methods.

Which one may affect your use case?

Targets
    Occurrences of 'synchronized' in Project
Found Occurrences in Project  (25 usages found)
    Unclassified  (25 usages found)
        jSerialComm.main  (25 usages found)
            com.fazecast.jSerialComm  (25 usages found)
                SerialPort.java  (25 usages found)
                    addShutdownHook(Thread)  (1 usage found)
                        348 static public synchronized void addShutdownHook(Thread hook) { shutdownHooks.add(hook); }
                    getCommPorts()  (1 usage found)
                        372 static public synchronized SerialPort[] getCommPorts() { return getCommPortsNative(); }
                    getCommPort(String)  (1 usage found)
                        384 static public synchronized SerialPort getCommPort(String portDescriptor) throws SerialPortInvalidPortException
                    openPort(int, int, int)  (1 usage found)
                        452 public final synchronized boolean openPort(int safetySleepTime, int deviceSendQueueSize, int deviceReceiveQueueSize)
                    openPort(int)  (1 usage found)
                        517 public final synchronized boolean openPort(int safetySleepTime) { return openPort(safetySleepTime, sendDeviceQueueSize, receiveDeviceQueueSize); }
                    openPort()  (1 usage found)
                        533 public final synchronized boolean openPort() { return openPort(0); }
                    closePort()  (1 usage found)
                        542 public final synchronized boolean closePort()
                    isOpen()  (1 usage found)
                        559 public final synchronized boolean isOpen() { return (portHandle != 0); }
                    disablePortConfiguration()  (1 usage found)
                        568 public final synchronized void disablePortConfiguration() { disableConfig = true; }
                    disableExclusiveLock()  (1 usage found)
                        576 public final synchronized void disableExclusiveLock() { disableExclusiveLock = true; }
                    allowElevatedPermissionsRequest()  (1 usage found)
                        599 public final synchronized void allowElevatedPermissionsRequest() { requestElevatedPermissions = true; }
                    getLastErrorLocation()  (1 usage found)
                        610 public final synchronized int getLastErrorLocation() { return getLastErrorLocation(portHandle); }
                    getLastErrorCode()  (1 usage found)
                        620 public final synchronized int getLastErrorCode() { return getLastErrorCode(portHandle); }
                    addDataListener(SerialPortDataListener)  (1 usage found)
                        890 public final synchronized boolean addDataListener(SerialPortDataListener listener)
                    removeDataListener()  (1 usage found)
                        913 public final synchronized void removeDataListener()
                    flushIOBuffers()  (1 usage found)
                        1000 public final synchronized boolean flushIOBuffers()
                    setComPortParameters(int, int, int, int, boolean)  (1 usage found)
                        1079 public final synchronized boolean setComPortParameters(int newBaudRate, int newDataBits, int newStopBits, int newParity, boolean useRS485Mode)
                    setComPortTimeouts(int, int, int)  (1 usage found)
                        1148 public final synchronized boolean setComPortTimeouts(int newTimeoutMode, int newReadTimeout, int newWriteTimeout)
                    setBaudRate(int)  (1 usage found)
                        1178 public final synchronized boolean setBaudRate(int newBaudRate)
                    setNumDataBits(int)  (1 usage found)
                        1199 public final synchronized boolean setNumDataBits(int newDataBits)
                    setNumStopBits(int)  (1 usage found)
                        1227 public final synchronized boolean setNumStopBits(int newStopBits)
                    setFlowControl(int)  (1 usage found)
                        1277 public final synchronized boolean setFlowControl(int newFlowControlSettings)
                    setParity(int)  (1 usage found)
                        1305 public final synchronized boolean setParity(int newParity)
                    setRs485ModeParameters(boolean, boolean, boolean, boolean, int, int)  (1 usage found)
                        1360 public final synchronized boolean setRs485ModeParameters(boolean useRS485Mode, boolean rs485RtsActiveHigh, boolean enableTermination
                    setXonXoffCharacters(byte, byte)  (1 usage found)
                        1389 public final synchronized boolean setXonXoffCharacters(byte xonStartCharacter, byte xoffStopCharacter)

Besides, I think related PRs are always welcome ;)
(Although unfortunatelly I haven't seen much activity in this project recently :( )

I thought the author of the project would reply and could give me a short answer. I was not asking others to search in code and do analysis for me. I think another person would need much time then the one who wrote the code.

Thanks for your help and support @NorbertSandor, it's very much appreciated!

@xberkhout, I haven't done any research whatsoever on the new Java Virtual Thread support; however, replacing my synchronized blocks with ReentrantLocks is trivial (and probably does scale better than using internal synchronization). I'll make this change and check it in shortly.

Thank you Will.

The most important parts to replace are the wait/notify inside synchronized blocks/methods. It is not directly necessary to replace synchronized around serial properties, but it will not do any harm either. The streaming of data itself is the most important part, but I would suggest to change them all to keep it consistent.

This has been done. It is present in the following beta library version, which seems to work without issue for me:

https://www.dropbox.com/t/wOjW1tbTqZNcHLvX

Thank you very much for the quick response. I just wanted to mention it so it would be ready for the release of Java 21 in September, but already a beta now is great.

I will put the new jar in the build process for my beta software and see what happens. It will take some weeks to get serious feedback.

Fully implemented with the latest release of v2.10.0 and v2.10.1. Going to close this for now, please open a new issue if there are further problems in the future. Thanks!

I have positive feedback. It works well with virtual threads in my application, while the former 2.9.3 gave some issues. So good all good for now. Needs more testing with more users, but it looks good.