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

worker: refactor thread id management #25796

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

  • Assign thread IDs to Environment instances, rather than Workers.
    This is more embedder-friendly than the current system, in which
    all “main threads” (if there are multiple ones) would get the
    id 0.
  • Because that means that isMainThread === (threadId === 0) no longer
    holds, refactor isMainThread into a separate entity. Implement it
    in a way that allows for future extensibility, because we use
    isMainThread in multiple different ways (determining whether there
    is a parent thread; determining whether the current thread has control
    of the current process; etc.).
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@addaleax addaleax added the worker Issues and PRs related to Worker support. label Jan 29, 2019
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 29, 2019
src/env.h Show resolved Hide resolved
Debug(this, "Set up worker with id %llu", thread_id_);
wrap->Set(env->context(),
env->thread_id_string(),
Number::New(env->isolate(), static_cast<double>(thread_id_)))
Copy link
Member

Choose a reason for hiding this comment

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

Now come to think of it, maybe this should be a BigInt?

Copy link
Member Author

Choose a reason for hiding this comment

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

@nodejs/workers Thoughts? BigInt seems like it might be a bit much, but since it’s really just an identifier, that might be okay?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds OK to me

Copy link
Member

Choose a reason for hiding this comment

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

In order to be relevant that'd require spawning more than 9,007,199,254,740,991 threads.

If we can spawn 1000 threads every seconds (a lot) and our program does nothing but spawn threads and we disregard any other limit or cleanup - it would take us ~285,616 years to exhaust that number of threads and overflow.

9007199254740991 / 1000 (threads per second) / 60 (seconds per minute) / 60 (minutes per hour) / 24 (hours per day) / 365 (days per year) = ~285616

If our machine is super fast and we spawn 100,000 threads per second instead, it would still take us over 2000 years. I certainly hope no one is going to run a Node.js server for that long.

I think a double is fine 😅

@addaleax
Copy link
Member Author

Rebased due to merge conflicts, CI: https://ci.nodejs.org/job/node-test-pull-request/20418/

- Assign thread IDs to `Environment` instances, rather than Workers.
  This is more embedder-friendly than the current system, in which
  all “main threads” (if there are multiple ones) would get the
  id `0`.
- Because that means that `isMainThread === (threadId === 0)` no longer
  holds, refactor `isMainThread` into a separate entity. Implement it
  in a way that allows for future extensibility, because we use
  `isMainThread` in multiple different ways (determining whether there
  is a parent thread; determining whether the current thread has control
  of the current process; etc.).
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2019
@addaleax
Copy link
Member Author

addaleax commented Feb 1, 2019

Sigh … rebased again.

CI: https://ci.nodejs.org/job/node-test-pull-request/20511/

@addaleax
Copy link
Member Author

addaleax commented Feb 1, 2019

Landed in 393c196

This depends on #25541 in terms of backporting.

@addaleax addaleax closed this Feb 1, 2019
@addaleax addaleax deleted the thread-id-generic branch February 1, 2019 19:24
addaleax added a commit that referenced this pull request Feb 1, 2019
- Assign thread IDs to `Environment` instances, rather than Workers.
  This is more embedder-friendly than the current system, in which
  all “main threads” (if there are multiple ones) would get the
  id `0`.
- Because that means that `isMainThread === (threadId === 0)` no longer
  holds, refactor `isMainThread` into a separate entity. Implement it
  in a way that allows for future extensibility, because we use
  `isMainThread` in multiple different ways (determining whether there
  is a parent thread; determining whether the current thread has control
  of the current process; etc.).

PR-URL: #25796
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
targos pushed a commit that referenced this pull request Feb 10, 2019
- Assign thread IDs to `Environment` instances, rather than Workers.
  This is more embedder-friendly than the current system, in which
  all “main threads” (if there are multiple ones) would get the
  id `0`.
- Because that means that `isMainThread === (threadId === 0)` no longer
  holds, refactor `isMainThread` into a separate entity. Implement it
  in a way that allows for future extensibility, because we use
  `isMainThread` in multiple different ways (determining whether there
  is a parent thread; determining whether the current thread has control
  of the current process; etc.).

PR-URL: #25796
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@targos targos mentioned this pull request Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants