Trigger the event loop semaphore at the right time#6
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This should prevent the hidden window on Windows from getting spammed, and also prevent qode from entering this weird "slow down" state.
sedwards2009 commented Jun 24, 2022
The next task is to see if this affects Linux and to continue dog food testing this PR. |
sedwards2009 commented Jun 25, 2022
Someone over at NodeGui also did some testing of the event loop performance: nodegui/nodegui#884 They were on an M1 mac which would have been running NodeGui via emulation. |
sedwards2009 commented Jun 30, 2022
@a7ul 👇 |
| // Tell the worker thread to continue polling. | ||
| uv_sem_post(&embed_sem_); |
There was a problem hiding this comment.
Yes, it has moved down to WakeupMainThread().
This is the most important part of the bug fix which affected Windows. UvRunOnce() gets called from two different places/cases. In one case, you want the semaphore to be touched (Qt event loop is running), and in the other case you don't (no Qt event loop). Hitting the semaphore without the Qt event loop was screwing its counter up.
| // Wait for the main loop to deal with events. | ||
| uv_sem_wait(&self->embed_sem_); | ||
| } |
There was a problem hiding this comment.
If we skip the batch processing and only do this change where we wait for main loop to deal with events after main thread has woken up does it achive similar results?
If not lets go with batch processing.
There was a problem hiding this comment.
There are two mostly separate things in this PR:
- The moving of the semaphore is the most important part here as it fixes Windows.
- Processing multiple UV events each time (batch).
The batch is optional, but the impact on performance is quite great if you are doing I/O. I remember someone complaining in an issue somewhere that NodeGui's I/O is much much slower than plain nodejs.
(Yes, I probably should've made 2 PRs. my bad)
sedwards2009 commented Jul 1, 2022
@a7ul I'm going to need you to hit the Merge button. I don't have the rights. |
a7ul commented Jul 1, 2022
I had invited you to the entire org actually. I ll merge this one though |
a7ul commented Jul 1, 2022
This is released now |
2 participants
This should prevent the hidden window on Windows from getting spammed,
and also prevent qode from entering this weird "slow down" state.
issue #4
It also contains some improvements to drastically increase the I/O throughput on Windows. Basically, triggering the main thread to go process nodejs events is expensive. The change is to call
uv_run()multiple times instead of just once whenever we need to process nodejs events. When I/O is busy, calling multiple times will process more I/O events with less of the overhead and latency incurred by the two threads communicating with each other.I have a little test program which times writing out the numbers 1 to 1000000 to a file one line at a time. Afterwards, it reads the file back in using 1KB chunks. The result are below are for Windows and Linux. They include plain nodejs, Qode without the extra event processing, and also Qode with different amounts of extra
uv_run()calls.Note: The amount of time processing a batch of events is capped at 8ms to prevent high I/O conditions from starving off the Qt event loop.
Windows
Linux
Note: Different machines are used between Windows and Linux.
Around about 16 extra calls in a batch seems to be a good trade off for Windows and Linux. I assume that this also holds for macOS.