timers: allow Immediates to be unrefed by apapirovski · Pull Request #18139 · nodejs/node · GitHub

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

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.

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/

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.

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>

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

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 addaleax and jasnell 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 outstanding review comments, 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

timers: allow Immediates to be unrefed by apapirovski · Pull Request #18139 · nodejs/node · GitHub

Uh oh!

Uh oh!

timers: allow Immediates to be unrefed - #18139

timers: allow Immediates to be unrefed#18139
apapirovski wants to merge 4 commits into
nodejs:masterfrom
apapirovski:feature-set-immediate-unref

apapirovski commented Jan 14, 2018 •
edited
Loading

Uh oh!

Uh oh!

fhinkel Jan 14, 2018

Uh oh!

apapirovski Jan 15, 2018

Uh oh!

fhinkel Jan 15, 2018

Uh oh!

apapirovski commented Jan 15, 2018 •
edited
Loading

Uh oh!

Uh oh!

jasnell Jan 15, 2018

Uh oh!

apapirovski Jan 15, 2018 •
edited
Loading

Uh oh!

Uh oh!

jasnell Jan 15, 2018

Uh oh!

apapirovski commented Jan 18, 2018

Uh oh!

evanlucas commented Jan 30, 2018

Uh oh!

AndreasMadsen commented Feb 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

timers: allow Immediates to be unrefed by apapirovski · Pull Request #18139 · nodejs/node · GitHub

Uh oh!

Uh oh!

timers: allow Immediates to be unrefed - #18139

timers: allow Immediates to be unrefed#18139apapirovski wants to merge 4 commits intonodejs:masternodejs/node:masterfrom apapirovski:feature-set-immediate-unrefapapirovski/node:feature-set-immediate-unrefCopy head branch name to clipboard

Conversation

apapirovski commented Jan 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

fhinkel Jan 14, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovski Jan 15, 2018

Choose a reason for hiding this comment

Uh oh!

fhinkel Jan 15, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell Jan 15, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovski Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell Jan 15, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Jan 18, 2018

Uh oh!

evanlucas commented Jan 30, 2018

Uh oh!

AndreasMadsen commented Feb 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

timers: allow Immediates to be unrefed#18139
apapirovski wants to merge 4 commits into
nodejs:masterfrom
apapirovski:feature-set-immediate-unref

apapirovski commented Jan 14, 2018 •
edited
Loading

apapirovski commented Jan 15, 2018 •
edited
Loading

apapirovski Jan 15, 2018 •
edited
Loading