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

timers: allow Immediates to be unrefed #18139

Closed

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented Jan 14, 2018

Slightly refactor the Immediates handling to allow for them to be unrefed, similar to setTimeout, but without extra handles. Adds immediate.ref() and immediate.unref().

There's a roughly 5-10% performance regression on some of the immediate benchmarks, as well as a more substantial regression on clearImmediate. Both are a result of needing to track an extra property on the Immediate class. That said, since 9.0.0 got released we've seen the performance on setImmediate benchmarks grow by roughly 40-50%, so IMO we can afford this slight regression.

CI: https://ci.nodejs.org/job/node-test-pull-request/12523/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/96/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1202/

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

timers

@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 14, 2018
@apapirovski apapirovski added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. semver-minor PRs that contain new features and should be released in the next minor version. labels Jan 14, 2018
added: REPLACEME
-->

When called, requests that the Node.js event loop *not* exit so long as the
Copy link
Member

Choose a reason for hiding this comment

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

I have a hard time understanding this sentence. Could you re-write it with simpler grammar? 🙏

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't mind rewriting but the current version is basically copy & pasted from Timers. If I'm doing that, I would prefer to do both in the same PR — if you don't mind?

Copy link
Member

Choose a reason for hiding this comment

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

That's fine then!

Slightly refactor the immediates handling to allow for them to be
unrefed, similar to setTimeout, but without extra handles.

Document the new `immediate.ref()` and `immediate.unref()` methods.
@apapirovski
Copy link
Member Author

apapirovski commented Jan 15, 2018

Some changes to the performance profile here and also accounting for a few more edge cases around clearImmediate & ref/unref combination.

I need to write a few more tests before this lands but not sure when I'll get around to them.

CI: https://ci.nodejs.org/job/node-test-pull-request/12529/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/98/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1203/

src/env-inl.h Outdated
@@ -538,16 +550,27 @@ inline void Environment::set_fs_stats_field_array(double* fields) {

void Environment::SetImmediate(native_immediate_callback cb,
void* data,
v8::Local<v8::Object> obj) {
v8::Local<v8::Object> obj,
bool ref) {
Copy link
Member

Choose a reason for hiding this comment

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

Rather than a bool, I'd prefer an enum type argument here.

Copy link
Member Author

@apapirovski apapirovski Jan 15, 2018

Choose a reason for hiding this comment

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

I didn't want that argument to be used, to be honest (it was solely there to not have to replicate the code within SetUnrefImmediate). Would something like this address the concern (with CreateImmediate being private)? Or would you still prefer enum in this situation?

void Environment::CreateImmediate(native_immediate_callback cb,
                               void* data,
                               v8::Local<v8::Object> obj,
                               bool ref) {
  native_immediate_callbacks_.push_back({
    cb,
    data,
    std::unique_ptr<v8::Persistent<v8::Object>>(obj.IsEmpty() ?
        nullptr : new v8::Persistent<v8::Object>(isolate_, obj)),
    ref
  });
  immediate_info()->count_inc(1);
}

void Environment::SetImmediate(native_immediate_callback cb,
                               void* data,
                               v8::Local<v8::Object> obj) {
  CreateImmediate(cb, data, obj, true);

  if (immediate_info()->ref_count() == 0)
    ToggleImmediateRef(true);
  immediate_info()->ref_count_inc(1);
}

void Environment::SetUnrefImmediate(native_immediate_callback cb,
                                    void* data,
                                    v8::Local<v8::Object> obj) {
  CreateImmediate(cb, data, obj, false);
}

Copy link
Member

Choose a reason for hiding this comment

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

Since CreateImmediate() is private, it's not a big deal. Just a preference.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 18, 2018
@apapirovski
Copy link
Member Author

Thanks everyone! Landed in c123467

@apapirovski apapirovski deleted the feature-set-immediate-unref branch January 18, 2018 20:56
apapirovski added a commit that referenced this pull request Jan 18, 2018
Refactor Immediates handling to allow for them to be unrefed, similar
to setTimeout, but without extra handles.

Document the new `immediate.ref()` and `immediate.unref()` methods.

Add SetImmediateUnref on the C++ side.

PR-URL: #18139
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@evanlucas
Copy link
Contributor

This does not land cleanly on v9.x-staging. Can you submit a backport PR?

@AndreasMadsen
Copy link
Member

Backported in #18488

apapirovski added a commit to apapirovski/node that referenced this pull request Feb 26, 2018
Refactor Immediates handling to allow for them to be unrefed, similar
to setTimeout, but without extra handles.

Document the new `immediate.ref()` and `immediate.unref()` methods.

Add SetImmediateUnref on the C++ side.

PR-URL: nodejs#18139
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@apapirovski apapirovski added the notable-change PRs with changes that should be highlighted in changelogs. label Feb 26, 2018
addaleax pushed a commit that referenced this pull request Feb 26, 2018
Refactor Immediates handling to allow for them to be unrefed, similar
to setTimeout, but without extra handles.

Document the new `immediate.ref()` and `immediate.unref()` methods.

Add SetImmediateUnref on the C++ side.

Backport-PR-URL: #19006
PR-URL: #18139
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
Refactor Immediates handling to allow for them to be unrefed, similar
to setTimeout, but without extra handles.

Document the new `immediate.ref()` and `immediate.unref()` methods.

Add SetImmediateUnref on the C++ side.

Backport-PR-URL: #19006
PR-URL: #18139
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
Refactor Immediates handling to allow for them to be unrefed, similar
to setTimeout, but without extra handles.

Document the new `immediate.ref()` and `immediate.unref()` methods.

Add SetImmediateUnref on the C++ side.

Backport-PR-URL: #19006
PR-URL: #18139
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Feb 27, 2018
Notable Changes:

* **libuv**:
  - Updated to libuv 1.19.2 (Colin Ihrig) [#18918](#18918)

* **src**:
  - Add initial support for Node.js-specific post-mortem metadata (Matheus Marchini) [#14901](#14901)

* **timers**:
  - The return value of `setImmediate()` now has `ref()` and `unref()` methods (Anatoli Papirovski) [#18139](#18139)

* **util**:
  - It is now possible to get the name for a numerical platform-specific error code as a string (Joyee Cheung) [#18186](#18186)
@addaleax addaleax mentioned this pull request Feb 27, 2018
rvagg added a commit that referenced this pull request Mar 1, 2018
Notable Changes:

* **libuv**:
  - Updated to libuv 1.19.2 (Colin Ihrig) [#18918](#18918)

* **src**:
  - Add initial support for Node.js-specific post-mortem metadata (Matheus Marchini) [#14901](#14901)

* **timers**:
  - The return value of `setImmediate()` now has `ref()` and `unref()` methods (Anatoli Papirovski) [#18139](#18139)

* **util**:
  - It is now possible to get the name for a numerical platform-specific error code as a string (Joyee Cheung) [#18186](#18186)

PR-URL: #19040
Prepared-By: Anna Henningsen <anna@addaleax.net>
rvagg added a commit that referenced this pull request Mar 1, 2018
Notable Changes:

* **libuv**:
  - Updated to libuv 1.19.2 (Colin Ihrig) [#18918](#18918)

* **src**:
  - Add initial support for Node.js-specific post-mortem metadata (Matheus Marchini) [#14901](#14901)

* **timers**:
  - The return value of `setImmediate()` now has `ref()` and `unref()` methods (Anatoli Papirovski) [#18139](#18139)

* **util**:
  - It is now possible to get the name for a numerical platform-specific error code as a string (Joyee Cheung) [#18186](#18186)

PR-URL: #19040
Prepared-By: Anna Henningsen <anna@addaleax.net>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 12, 2018
Refactor Immediates handling to allow for them to be unrefed, similar
to setTimeout, but without extra handles.

Document the new `immediate.ref()` and `immediate.unref()` methods.

Add SetImmediateUnref on the C++ side.

PR-URL: nodejs#18139
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
Refactor Immediates handling to allow for them to be unrefed, similar
to setTimeout, but without extra handles.

Document the new `immediate.ref()` and `immediate.unref()` methods.

Add SetImmediateUnref on the C++ side.

Backport-PR-URL: #19265
PR-URL: #18139
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Refactor Immediates handling to allow for them to be unrefed, similar
to setTimeout, but without extra handles.

Document the new `immediate.ref()` and `immediate.unref()` methods.

Add SetImmediateUnref on the C++ side.

PR-URL: nodejs#18139
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Notable Changes:

* **libuv**:
  - Updated to libuv 1.19.2 (Colin Ihrig) [nodejs#18918](nodejs#18918)

* **src**:
  - Add initial support for Node.js-specific post-mortem metadata (Matheus Marchini) [nodejs#14901](nodejs#14901)

* **timers**:
  - The return value of `setImmediate()` now has `ref()` and `unref()` methods (Anatoli Papirovski) [nodejs#18139](nodejs#18139)

* **util**:
  - It is now possible to get the name for a numerical platform-specific error code as a string (Joyee Cheung) [nodejs#18186](nodejs#18186)

PR-URL: nodejs#19040
Prepared-By: Anna Henningsen <anna@addaleax.net>
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. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants