Skip to content

Trigger the event loop semaphore at the right time #6

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.

Sign up for GitHub

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

Merged
a7ul merged 3 commits into from
Jul 1, 2022
Merged

Trigger the event loop semaphore at the right time #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9e244ac
Trigger the event loop semaphore at the right time
sedwards2009 Jun 22, 2022
2325dc3
Speed up I/O processing by processing batches of events
sedwards2009 Jun 24, 2022
41c633c
Tweak the size of a batch of uv_run() calls
sedwards2009 Jun 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions qode/integration/node_integration.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ NodeIntegration::NodeIntegration()
embed_closed_(false) {
}

constexpr uint64_t EVENT_BATCH_TIMEOUT_MS = 8;
constexpr int EVENT_BATCH_SIZE = 16;

NodeIntegration::~NodeIntegration() {
// Quit the embed thread.
embed_closed_ = true;
Expand Down Expand Up @@ -63,9 +66,6 @@ void NodeIntegration::UvRunOnce() {

// Deal with uv events.
uv_run(uv_loop_, UV_RUN_NOWAIT);

// Tell the worker thread to continue polling.
uv_sem_post(&embed_sem_);
Comment on lines -66 to -68

@a7ul a7ul Jun 30, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change intentional? 🤔

@sedwards2009 sedwards2009 Jun 30, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

void NodeIntegration::CallNextTick() {
Expand All @@ -79,6 +79,19 @@ void NodeIntegration::ReleaseHandleRef() {
void NodeIntegration::WakeupMainThread() {
PostTask([this] {
this->UvRunOnce();
// ^ This also updates the uv_now() timestamp.
uint64_t start_time_ms = uv_now(uv_loop_);

int loop_count = EVENT_BATCH_SIZE;
uint64_t elapsed_ms = 0;
while (loop_count != 0 && (elapsed_ms < EVENT_BATCH_TIMEOUT_MS)) {
loop_count--;
this->UvRunOnce();
elapsed_ms = uv_now(uv_loop_) - start_time_ms;
}

// Tell the worker thread to continue polling.
uv_sem_post(&this->embed_sem_);
});
}

Expand All @@ -91,8 +104,6 @@ void NodeIntegration::EmbedThreadRunner(void *arg) {
NodeIntegration* self = static_cast<NodeIntegration*>(arg);

while (true) {
// Wait for the main loop to deal with events.
uv_sem_wait(&self->embed_sem_);
if (self->embed_closed_)
break;

Expand All @@ -113,6 +124,9 @@ void NodeIntegration::EmbedThreadRunner(void *arg) {

// Deal with event in main thread.
self->WakeupMainThread();

// Wait for the main loop to deal with events.
uv_sem_wait(&self->embed_sem_);
}
Comment on lines +127 to 130

@a7ul a7ul Jun 30, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sedwards2009 sedwards2009 Jun 30, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@a7ul a7ul Jun 30, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! that make sense.

}

Expand Down
7 changes: 6 additions & 1 deletion qode/integration/node_integration_win.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "qode/integration/node_integration_win.h"
#include "uv/src/uv-common.h"
#include <stdio.h>

// http://blogs.msdn.com/oldnewthing/archive/2004/10/25/247180.aspx
extern "C" IMAGE_DOS_HEADER __ImageBase;
Expand Down Expand Up @@ -70,7 +71,11 @@ void NodeIntegrationWin::PostTask(const std::function<void()>& task) {
::EnterCriticalSection(&lock_);
tasks_[++task_id_] = task;
::LeaveCriticalSection(&lock_);
::PostMessage(message_window_, WM_USER, task_id_, 0L);
BOOL result = ::PostMessage(message_window_, WM_USER, task_id_, 0L);
if (result == 0) {
int lastError = ::GetLastError();
fprintf(stderr, "Qode: PostMessage() failed! LastError=%d\n", lastError);
}
}

void NodeIntegrationWin::OnTask(int id) {
Expand Down