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

src: UV_RUN_ONCE in a loop is inefficient #8496

Closed
bnoordhuis opened this issue Sep 12, 2016 · 3 comments
Closed

src: UV_RUN_ONCE in a loop is inefficient #8496

bnoordhuis opened this issue Sep 12, 2016 · 3 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. lib / src Issues and PRs related to general changes in the lib or src directory.

Comments

@bnoordhuis
Copy link
Member

src/node.cc calls uv_run(loop, UV_RUN_ONCE) in a loop:

do {
  v8_platform.PumpMessageLoop(isolate);
  more = uv_run(env.event_loop(), UV_RUN_ONCE);

  if (more == false) {
    v8_platform.PumpMessageLoop(isolate);
    EmitBeforeExit(&env);

    // Emit `beforeExit` if the loop became alive either after emitting
    // event, or after running some callbacks.
    more = uv_loop_alive(env.event_loop());
    if (uv_run(env.event_loop(), UV_RUN_NOWAIT) != 0)
      more = true;
  }
} while (more == true);

uv_run() looks like this:

int uv_run(uv_loop_t* loop, uv_run_mode mode) {
  // ...
  while (r != 0 && loop->stop_flag == 0) {
    uv__update_time(loop);
    uv__run_timers(loop);
    // ...
    uv__io_poll(loop, timeout);
    uv__run_check(loop);
    uv__run_closing_handles(loop);

    if (mode == UV_RUN_ONCE) {
      uv__update_time(loop);
      uv__run_timers(loop);
    }

    r = uv__loop_alive(loop);
    if (mode == UV_RUN_ONCE || mode == UV_RUN_NOWAIT)
      break;
  }
  // ...
}

The way we use UV_RUN_ONCE is:

  1. Inefficient. uv__update_time() is called twice. It's expensive on systems where querying the system time is expensive (e.g. virtualized systems.)
  2. Arguably incorrect. Timers are effectively dispatched twice per look tick.

Branched off #8460 (comment).

@bnoordhuis bnoordhuis added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 12, 2016
@Fishrock123
Copy link
Contributor

This sounds like it has an awfully good chance of being related to #3665

@Trott
Copy link
Member

Trott commented Jul 10, 2017

@bnoordhuis Any new information to add to this? If no one's actively working on it, maybe we can add a help wanted label?

@Trott Trott added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 10, 2017
@bnoordhuis bnoordhuis added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Jul 10, 2017
@bnoordhuis
Copy link
Member Author

Good idea. Label added.

addaleax pushed a commit that referenced this issue Aug 17, 2017
Node.js currently uses the V8 implementation of the DefaultPlatform
which schedules VM tasks on a V8 managed thread pool. Since the Node.js
event loop is not aware of these tasks, the Node.js process may exit
while there are outstanding VM tasks. This will become problematic once
asynchronous wasm compilation lands in V8.

This PR introduces a Node.js specific implementation of the v8::Platform
on top of libuv so that the event loop is aware of outstanding VM tasks.

PR-URL: #14001
Fixes: #3665
Fixes: #8496
Fixes: #12980
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 10, 2017
Node.js currently uses the V8 implementation of the DefaultPlatform
which schedules VM tasks on a V8 managed thread pool. Since the Node.js
event loop is not aware of these tasks, the Node.js process may exit
while there are outstanding VM tasks. This will become problematic once
asynchronous wasm compilation lands in V8.

This PR introduces a Node.js specific implementation of the v8::Platform
on top of libuv so that the event loop is aware of outstanding VM tasks.

PR-URL: #14001
Fixes: #3665
Fixes: #8496
Fixes: #12980
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 12, 2017
Node.js currently uses the V8 implementation of the DefaultPlatform
which schedules VM tasks on a V8 managed thread pool. Since the Node.js
event loop is not aware of these tasks, the Node.js process may exit
while there are outstanding VM tasks. This will become problematic once
asynchronous wasm compilation lands in V8.

This PR introduces a Node.js specific implementation of the v8::Platform
on top of libuv so that the event loop is aware of outstanding VM tasks.

PR-URL: #14001
Fixes: #3665
Fixes: #8496
Fixes: #12980
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

No branches or pull requests

3 participants