timers: allow Immediates to be unrefed #18139

apapirovski · 2018-01-14T03:14:16Z

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

fhinkel · 2018-01-14T09:32:26Z

doc/api/timers.md

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


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

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?

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 · 2018-01-15T00:13:41Z

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/

jasnell · 2018-01-15T15:59:39Z

src/env-inl.h

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


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

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

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

apapirovski · 2018-01-18T20:56:37Z

Thanks everyone! Landed in c123467

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 · 2018-01-30T16:47:53Z

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

AndreasMadsen · 2018-02-20T23:04:30Z

Backported in #18488

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>

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>

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)

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>

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>

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>

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>

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>

apapirovski requested review from jasnell and addaleax January 14, 2018 03:14

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 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

apapirovski force-pushed the feature-set-immediate-unref branch from 61b648a to 3a3c628 Compare January 14, 2018 06:06

fhinkel reviewed Jan 14, 2018

View reviewed changes

addaleax approved these changes Jan 14, 2018

View reviewed changes

timers: allow Immediates to be unrefed

83ff89b

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 force-pushed the feature-set-immediate-unref branch from 3a3c628 to 83ff89b Compare January 14, 2018 23:41

fixup: improve performance

e8d5b4c

fhinkel approved these changes Jan 15, 2018

View reviewed changes

fixup: additional test & ref/unref edge case handling

a2d51a9

jasnell reviewed Jan 15, 2018

View reviewed changes

fixup: abstract immediate creation into separate method

a0313b1

jasnell approved these changes Jan 15, 2018

View reviewed changes

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 closed this Jan 18, 2018

apapirovski deleted the feature-set-immediate-unref branch January 18, 2018 20:56

evanlucas added the backport-requested-v9.x label Jan 30, 2018

apapirovski mentioned this pull request Jan 31, 2018

[v9.x backport] 17738 and 17841 #18488

Closed

3 tasks

apapirovski added the notable-change PRs with changes that should be highlighted in changelogs. label Feb 26, 2018

addaleax added backported-to-v9.x and removed backport-requested-v9.x labels Feb 26, 2018

addaleax mentioned this pull request Feb 27, 2018

v9.7.0 proposal #19040

Closed

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

codebytere mentioned this pull request Nov 3, 2020

setImmediate crash electron/electron#26055

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: allow Immediates to be unrefed #18139

timers: allow Immediates to be unrefed #18139

apapirovski commented Jan 14, 2018 •

edited

fhinkel Jan 14, 2018

apapirovski Jan 15, 2018

fhinkel Jan 15, 2018

apapirovski commented Jan 15, 2018 •

edited

jasnell Jan 15, 2018

apapirovski Jan 15, 2018 •

edited

jasnell Jan 15, 2018

apapirovski commented Jan 18, 2018

evanlucas commented Jan 30, 2018

AndreasMadsen commented Feb 20, 2018

timers: allow Immediates to be unrefed #18139

timers: allow Immediates to be unrefed #18139

Conversation

apapirovski commented Jan 14, 2018 • edited

Checklist

Affected core subsystem(s)

fhinkel Jan 14, 2018

Choose a reason for hiding this comment

apapirovski Jan 15, 2018

Choose a reason for hiding this comment

fhinkel Jan 15, 2018

Choose a reason for hiding this comment

apapirovski commented Jan 15, 2018 • edited

jasnell Jan 15, 2018

Choose a reason for hiding this comment

apapirovski Jan 15, 2018 • edited

Choose a reason for hiding this comment

jasnell Jan 15, 2018

Choose a reason for hiding this comment

apapirovski commented Jan 18, 2018

evanlucas commented Jan 30, 2018

AndreasMadsen commented Feb 20, 2018

apapirovski commented Jan 14, 2018 •

edited

apapirovski commented Jan 15, 2018 •

edited

apapirovski Jan 15, 2018 •

edited