[windows] Use non blocking reads and writes

[reconbot]

We need help with this one!

• SerialPort Version: 5.0.0-beta6
• Operating System and Hardware Platform: Windows / All

Right now we can only support 3 or 4 ports open at a time in the same process on windows unless the env var UV_THREADPOOL_SIZE is set to something higher than 4. This is because we're using WriteFile and ReadFile for our reads and writes which are blocking. We should transition to WriteFileEx and ReadFileEx which support callbacks when data is read or written. This should allow us to support "any" number of ports and have lower cpu utilization.

Relevant code; EIO_Write and EIO_Read

[reconbot]

Notes

http://tinyclouds.org/iocp-links.html

The fundamental variation is that in a Unix you generally ask the kernel to wait for state change in a file descriptor's readability or writablity. With overlapped I/O and IOCPs the programmers waits for asynchronous function calls to complete.

Unix non-blocking I/O is not beautiful.

https://msdn.microsoft.com/en-us/library/ms679360(v=vs.85).aspx

The error codes returned by a function are not part of the Windows API specification and can vary by operating system or device driver. For this reason, we cannot provide the complete list of error codes that can be returned by each function. There are also many functions whose documentation does not include even a partial list of error codes that can be returned.

[noopkat]

not sure I can help out with the C++ side, but happy to manually test! Let me know 😁

[reconbot]

Async isn't always async. Dunno if it applies here. https://support.microsoft.com/en-us/help/156932/asynchronous-disk-i-o-appears-as-synchronous-on-windows

[Craytor]

General question regarding this - is it possible that this will help the CPU usage go below 30%? Maybe 5%-10%?

[reconbot]

Yes

[dustmop]

I have a working proof-of-concept using WriteFileEx and (incorrectly) CreateThread. Need to use uv_thread_id, implement the same for Read'ing, and clean it up. Will possibly have a PR by end of week?

[reconbot]

Woh! This is great!

Why do we need threads?

[dustmop]

From WriteFileEx: https://msdn.microsoft.com/en-us/library/windows/desktop/aa365748(v=vs.85).aspx

"When this I/O operation finishes, and the calling thread is blocked in an alertable wait state, the operating system calls the function pointed to by lpCompletionRoutine"

An alertable wait state, from Alertable I/O: https://msdn.microsoft.com/en-us/library/windows/desktop/aa363772(v=vs.85).aspx

"To do this, it must enter an alertable state. A thread can only do this by calling one of the following functions with the appropriate flags: ..."

The WriteFileEx calls needs to be on a thread that can be blocked (with something like SleepEx), otherwise the OS won't call the async callback. Perhaps there's something I'm missing with regards to how node's events work, but I think the only way to do this is use a thread from the uv pool. Hacking on it today, I couldn't get the callback to ever trigger with everything staying on the main event loop thread. Hope this doesn't conflict with the expected utilization and performance improvements.

[dustmop]

Actually, since the libuv threadpool only allows 3 or 4 threads, as mentioned a few times in this thread (sorry!) perhaps the correct solution is to actually use CreateThread for the WriteFileEx call and callback. That way, the libuv pool won't be exhausted. A new thread will be created for each write operation, which I don't understand the consequences of quite that well, but it doesn't seem possible to avoid it.

[reconbot]

There will only ever be a single write per port going on at one time. I don't know if that helps.

…

--- Francis Gulotta wizard@roborooter.com

On Tue, Aug 15, 2017 at 9:14 PM, Dustin Long ***@***.***> wrote: Actually, since the libuv threadpool only allows 3 or 4 threads, as mentioned a few times in this thread (sorry!) perhaps the correct solution is to actually use CreateThread for the WriteFileEx call and callback. That way, the libuv pool won't be exhausted. A new thread will be created for each write operation, which I don't understand the consequences of quite that well, but it doesn't seem possible to avoid it. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1221 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABlbkcCBodHNhnVOO_muusjIU_mPt63ks5sYmy2gaJpZM4OPBf1> .

[dustmop]

Update on my status:

1. Changing writing to use asynchronous callbacks is pretty simple. Using WriteFileEx and a FileIOCompletion callback just works, as long as its in a separate thread which can enter the alertable wait state.

2. Reading is much trickier. The reason is due to how Windows does buffering and timeouts. For starters, the comment on line 159 about setting "ReadIntervalTimeout" to "MAXDWORD" meaning "Never timeout" is wrong. MAXDWORD means "timeout immediately", so the Read* call returns right away, even if there's zero bytes in the buffer. This is the reason the code needs to poll constantly, pegging the cpu, and this will remain the case even using ReadFileEx's asynchronous callbacks. In my own testing, sending a two byte string from an Arduino to SerialPort every second, ReadFileEx was called ~150,000 times before receiving anything.

3. The fix for avoiding polling is to set "ReadIntervalTimeout" to 0, which is what actually means "never timeout". This will wait until the input buffer contains enough bytes to fulfill the requested size, but leads to another problem: the call will block if it doesn't have enough bytes, so the only safe technique is to request 1 byte at a time, which leads to lots of system calls receiving a single byte each.

4. I believe the best technique is to set ReadIntervalTimeout to 0, call ReadFileEx, then once the callback triggers, set ReadIntervalTimeout to MAXDWORD and call SetCommTimeouts again, before calling ReadFile to get the rest of what's in the buffer. This should be safe as long as there's one thread reading at a time, which I believe should be the case?

Another possible approach would be to have two separate HANDLEs, one for reading and one for writing, so that reading could use WaitForSingleObject to see if there's data ready on the input buffer. I haven't tested this yet to see if it could work; I will put it off until later as a separate change.

I've been figuring this out using a stand-alone command-line application, so will need to integrate into Serialport, which shouldn't be too difficult at this point.

[reconbot]

Wohhh! This is great!

• Yes, only one read per HANDLE will be in flight at a time. Same with writes.
• I'm unsure if we really really want to have an unlimited timeout, but maybe? Lets see how it works! Better than what we got for sure.

[reconbot]

Some notes from testing;

• Single device read and write low cpu 🎉
• Passes on binding read no more than x bytes fix(windows): Read at max bytesToRead #1322
• Passes on r/w for more 4+ arduinos
• Passes on integration 2k read/write
• Passes full Test suite

[dustmop]

Have some questions about the above testing results:

Passes on binding read no more than x bytes #1322

Should be fixed, now, if I understand correctly. I merged in the same fix from #1322 into the new PR.

Passes on r/w for more 4+ arduinos

I don't have a way to test this. Is this a regression? Does the current implementation handle it correctly?

Passes on integration 2k read/write

How do I run this?

Passes full Test suite

Is this the same as TEST_PORT=<port> npm test?

[reconbot]

The previous implementation would block the libuv thread pool which has 4 threads by default. So worst your case your implementation does the same.

full test suite is TEST_PORT=<port> npm test if you leave out the TEST_PORT it skips a ton of tests

[reconbot]

Keeping this open because we need to verify the # of concurrent arduinos we can run.

serialport@6.0.0-beta2 has been released! 🎉

[reconbot]

So far I haven't gotten support requests for this. It's passed my muster. I think it's worth going to prod.

I can't actually test how many concurrent connections we can make, it wouldn't be worse. We'll know for sure eventually. ¯\_(ツ)_/¯