Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better waitEvents implementation for *nix-like systems #78

Closed
wants to merge 7 commits into from

Conversation

graynk
Copy link

@graynk graynk commented Jan 30, 2021

Should close #53

@tresf
Copy link

tresf commented Jan 30, 2021

Thanks!

  • Please revert the compile lib, all of the unix ones need to be rebuilt after this change, so providing one architecture isn't needed. Furthemore, we build from older kernels so for consistency reasons, we'll do this with the next release.
  • I would caution against removing includes like #include <stdio.h>. This is probably safe because NULL is imported by time.h, but I would recommend reverting that change although it won't affect whether or not we merge it. I'm just not certain that all old and obscure compilers the project uses are compliant with the C specification so imports should be very carefully removed.

src/main/cpp/_nix_based/jssc.cpp Outdated Show resolved Hide resolved
src/main/java/jssc/SerialPort.java Outdated Show resolved Hide resolved
@graynk
Copy link
Author

graynk commented Feb 7, 2021

I put back #include <stdio.h> and I've got the libjssc.so from the latest master (couldn't just revert the commit)

@tresf
Copy link

tresf commented Feb 7, 2021

I put back #include <stdio.h> and I've got the libjssc.so from the latest master (couldn't just revert the commit)

This still seems to have modified the permissions, which shouldn't be needed.

@graynk
Copy link
Author

graynk commented Feb 7, 2021

This still seems to have modified the permissions, which shouldn't be needed.

Sorry about that, fixed.

@tresf
Copy link

tresf commented Jul 26, 2021

@graynk sorry for the delay, it appears this will likely be superseded by #90. I'd like to invite you to help review the new code.

@graynk
Copy link
Author

graynk commented Jul 27, 2021

@graynk sorry for the delay, it appears this will likely be superseded by #90. I'd like to invite you to help review the new code.

No problem. I looked at the code and left a couple of questions. Will close this now.

@graynk graynk closed this Jul 27, 2021
@centic9
Copy link

centic9 commented Aug 8, 2021

The changes in #90 adjusted "readBytes", but this PR was about adding polling in "waitEvents".

I hoped to see a reduction of CPU usage of the LinuxEventThread (which consumes up to 10% CPU on a Laptop for me) with this PR, but #90 does not seem to help there.

Would it be useful to apply the changes to waitEvents in this PR as well so the main event loop polls for events instead of busy-looping and sleeping?

@GMKennedy
Copy link
Collaborator

GMKennedy commented Aug 24, 2021

The changes in #90 adjusted "readBytes", but this PR was about adding polling in "waitEvents".

I hoped to see a reduction of CPU usage of the LinuxEventThread (which consumes up to 10% CPU on a Laptop for me) with this PR, but #90 does not seem to help there.

Would it be useful to apply the changes to waitEvents in this PR as well so the main event loop polls for events instead of busy-looping and sleeping?

@centic9 sorry for the late response on this, had to do quite a bit of background research. Could you give us a little more information on your test environment and what you are using to profile the cpu spikes you are seeing? We still appear to be having some issues with opening the serial port in blocking mode (at least this appears to be the case)

@GMKennedy
Copy link
Collaborator

@graynk are you seeing performance issues with the most recent release version? How is your test environment configured?

@graynk
Copy link
Author

graynk commented Aug 24, 2021

@graynk are you seeing performance issues with the most recent release version? How is your test environment configured?

Sorry, actually I haven't been using this library for a long time now, I've moved on from that project and changed my employer since then.

@centic9
Copy link

centic9 commented Aug 24, 2021

@centic9 sorry for the late response on this, had to do quite a bit of background research. Could you give us a little more information on your test environment and what you are using to profile the cpu spikes you are seeing? We still appear to be having some issues with opening the serial port in blocking mode (at least this appears to be the case)

No problem, I have the following sample-application and the following impact on CPU (Intel(R) Core(TM) i7-8650U, Linux reports 8 entries in /proc/cpuinfo) when I run this locally:

  • Running with "current org.scream3r:jssc:2.8.0": Around 7-8% CPU usage
  • Removing the call to device.addEventListener() drops CPU to 0%
  • Running with "io.github.java-native:jssc:2.9.3": Similar 7-8% CPU usage
  • Running with a locally patched version which adds the poll and also increases the sleep in the loop (see below for actual patch): 0,2% CPU

For sampling CPU I simply use JVisualVM, it shows that the "EventThread" is using up the CPU in the first two cases and "LinuxEventThread.run()" is the method which consumes almost all the CPU.

Sample application:

     public class TM1638TestApp {
            public static void main(String[] args) throws IOException, InterruptedException {
                System.out.println("Starting Control");
                try (Control control = new Control()) {
                    System.out.println("Connecting");
                    control.connect("/dev/ttyUSB0");
        
                    for (int i = 0; i < 1000; i++) {
                        Thread.sleep(1000);
                    }
                }
            }
        
            public static class Control implements AutoCloseable {
                private SerialPort device;
        
                public void connect(String comPort) throws IOException {
                    System.out.println("Setting up communication with Arduino device via port " + comPort);
        
                    device = new SerialPort(comPort);
        
                    try {
                        device.openPort();
                        device.setParams(SerialPort.BAUDRATE_19200, SerialPort.DATABITS_8, SerialPort.STOPBITS_1, SerialPort.PARITY_NONE);
        
                        device.addEventListener(new Control.DeviceEventHandler());
                    } catch (SerialPortException e) {
                        close();
        
                        throw new IOException("While opening port " + comPort, e);
                    }
                }
        
                @Override
                public synchronized void close() throws IOException {
                    System.out.println("Shutting down Control");
        
                    if (device != null) {
                        try {
                            if(device.isOpened()) {
                                System.out.println("Close the device-port");
                                device.closePort();
                            }
                        } catch (SerialPortException e) {
                            throw new IOException(e);
                        }
                        device = null;
                    }
                }
        
                private static class DeviceEventHandler implements SerialPortEventListener {
                    @Override
                    public void serialEvent(SerialPortEvent event) {
                        // process the event
                    }
                }
            }
        }

Patch to add "poll" and adjust sleep in inner loop:

Index: src/main/cpp/_nix_based/jssc.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/main/cpp/_nix_based/jssc.cpp b/src/main/cpp/_nix_based/jssc.cpp
--- a/src/main/cpp/_nix_based/jssc.cpp	(revision 003dd4f31b749f0c0c151203742c3b207526e15a)
+++ b/src/main/cpp/_nix_based/jssc.cpp	(revision 133da905073a79a62911f6981e010159ea26a164)
@@ -799,10 +799,15 @@
     jclass intClass = env->FindClass("[I");
     jobjectArray returnArray = env->NewObjectArray(sizeof(events)/sizeof(jint), intClass, NULL);
 
+    #ifdef POLLIN
+        struct pollfd waitingSet = { static_cast<int>(portHandle), POLLIN, 0 };
+        poll(&waitingSet, 1, 1000);
+    #endif
+
     /*Input buffer*/
     jint bytesCountIn = 0;
     ioctl(portHandle, FIONREAD, &bytesCountIn);
Index: src/main/java/jssc/SerialPort.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/main/java/jssc/SerialPort.java b/src/main/java/jssc/SerialPort.java
--- a/src/main/java/jssc/SerialPort.java	(revision 133da905073a79a62911f6981e010159ea26a164)
+++ b/src/main/java/jssc/SerialPort.java	(revision 5e45c8444de5bd1d465467c04d2bf660f3d15855)
@@ -1382,9 +1382,10 @@
                 try {
-                    Thread.sleep(0, 100);
+                    Thread.sleep(0, 500);
                 }
                 catch (Exception ex) {
                     //Do nothing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better waitEvents implementation for *nix-like systems
4 participants