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
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@apapirovski
Member

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

added: REPLACEME
-->
When called, requests that the Node.js event loop *not* exit so long as the

This comment has been minimized.

@fhinkel

fhinkel Jan 14, 2018

Member

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

This comment has been minimized.

@apapirovski

apapirovski Jan 15, 2018

Member

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?

This comment has been minimized.

@fhinkel

fhinkel Jan 15, 2018

Member

That's fine then!

apapirovski added some commits Jan 13, 2018

timers: allow Immediates to be unrefed
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

This comment has been minimized.

Member

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/

@@ -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) {

This comment has been minimized.

@jasnell

jasnell Jan 15, 2018

Member

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

This comment has been minimized.

@apapirovski

apapirovski Jan 15, 2018

Member

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);
}

This comment has been minimized.

@jasnell

jasnell Jan 15, 2018

Member

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

@apapirovski

This comment has been minimized.

Member

apapirovski commented Jan 18, 2018

Thanks everyone! Landed in c123467

@apapirovski apapirovski deleted the apapirovski:feature-set-immediate-unref branch Jan 18, 2018

apapirovski added a commit that referenced this pull request Jan 18, 2018

timers: allow Immediates to be unrefed
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

This comment has been minimized.

Member

evanlucas commented Jan 30, 2018

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

@apapirovski apapirovski referenced this pull request Jan 31, 2018

Closed

[v9.x backport] 17738 and 17841 #18488

3 of 3 tasks complete

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

timers: allow Immediates to be unrefed
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>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

timers: allow Immediates to be unrefed
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>
@AndreasMadsen

This comment has been minimized.

Member

AndreasMadsen commented Feb 20, 2018

Backported in #18488

apapirovski added a commit to apapirovski/node that referenced this pull request Feb 26, 2018

timers: allow Immediates to be unrefed
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>

addaleax added a commit that referenced this pull request Feb 26, 2018

timers: allow Immediates to be unrefed
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 26, 2018

timers: allow Immediates to be unrefed
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 added a commit that referenced this pull request Feb 26, 2018

timers: allow Immediates to be unrefed
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

2018-02-27, Version 9.7.0 (Current)
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 added a commit that referenced this pull request Feb 27, 2018

2018-02-27, Version 9.7.0 (Current)
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 referenced this pull request Feb 27, 2018

Closed

v9.7.0 proposal #19040

rvagg added a commit that referenced this pull request Mar 1, 2018

2018-03-01, Version 9.7.0 (Current)
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

2018-03-01, Version 9.7.0 (Current)
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 added a commit to gabrielschulhof/node that referenced this pull request Mar 15, 2018

timers: allow Immediates to be unrefed
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.

This applies the parts of c123467 that
are relevant to N-API.

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>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 27, 2018

timers: allow Immediates to be unrefed
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>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 27, 2018

timers: allow Immediates to be unrefed
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>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 28, 2018

timers: allow Immediates to be unrefed
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>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 4, 2018

timers: allow Immediates to be unrefed
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>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 6, 2018

timers: allow Immediates to be unrefed
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>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 6, 2018

timers: allow Immediates to be unrefed
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>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018

timers: allow Immediates to be unrefed
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>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 12, 2018

timers: allow Immediates to be unrefed
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 added a commit that referenced this pull request May 1, 2018

timers: allow Immediates to be unrefed
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 referenced this pull request May 2, 2018

Merged

v8.11.2 proposal #20478

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

timers: allow Immediates to be unrefed
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 added a commit to MayaLekova/node that referenced this pull request May 8, 2018

2018-03-01, Version 9.7.0 (Current)
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