process: add nice() function by zokker13 · Pull Request #21675 · nodejs/node

zokker13 · 2018-07-05T21:30:37Z

The nice function allows us to fine-tune the process to meet desired
scheduling behavior. If our process needs less schedule time because
it is a long-running one, we can increase the nice value and cause
the scheduler to select the process not so often.

If desired, here's the technical documentation: http://man7.org/linux/man-pages/man2/nice.2.html

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

addaleax

Looks pretty good so far … nice :)

addaleax · 2018-07-05T21:31:58Z

Can you use REPLACEME as the placeholder here? It gets picked up by release tooling automatically

addaleax · 2018-07-05T21:33:35Z

Can you use args[0].As<Int32>()->Value()? ->Int32Value() is an operation that potentially performs a type conversion, but that’s unnecessarily complex, since we already know that it’s an Int32

Sounds great and it's done, thanks!

addaleax · 2018-07-05T21:35:27Z

Can you inline typeof inc here? The engine is generally better at recognizing that pattern and can eliminate the temporary string you’d be creating here

Thanks for the deep insights. It is fixed.

addaleax · 2018-07-05T21:35:53Z

I think it’s a bit confusing to mix validation and the actual calls to _nice … could you separate them?

Split them up.:)

addaleax · 2018-07-05T21:37:32Z

Also: Your author name in this commit is given as “zokker13”. Is that intended or do you prefer to be listed (changelog, git log, AUTHORS file) with some other name? People typically prefer their full name, but ultimately it’s up to you.

Sign in to view

zokker13 · 2018-07-05T22:18:57Z

@addaleax Thanks for the review and the lightning fast response. Added your requests. Hope that "fixup" -commit is appropriate.

addaleax

Great, thanks!

devsnek · 2018-07-05T22:22:17Z

you can just combine this with the below
functionvalidateInc(inc){if(typeofinc==='number'){validateInt32(inc,'nice');}elseif(inc!==undefined){thrownewERR_INVALID_ARG_TYPE('nice',['number','undefined'],inc);}}

Nice catch, totally missed that!
Here it is: 655894e745de20829ff40ee330557da1305f9c7e

addaleax · 2018-07-05T22:25:03Z

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

As a heads up, our rules will require this PR to stay open for 72 hours so people have a chance to weigh in, and you may have to update this one or two times more during that time. But as first PRs go, this is definitely a first-class example. :)

Trott · 2018-07-05T22:40:54Z

This is a sentence fragment. You can make it a complete sentence by removing Though,.

For those not familiar with the 'nice' system, the two different uses of 'increase' here might be confusing. At the very least it would be a good idea to note that increasing the 'nice' value means lowering the process priority. This is hinted at in the paragraph above this one, but I still think using 'increase' back to back here could confuse some people.

I kinda changed the entire thing: a099642629b36e908c5114227c4fd8e9c18cc12f

Trott · 2018-07-05T22:42:18Z

Please remove **NOTE**:, the exclamation point, and use of you.
Maybe this?:
The nice value can be increased only. It can not be decreased.
...or...
The nice value can never be decreased except by the super user.
...or something like that.

Changed in a way that is (hopefully) better readable: a099642629b36e908c5114227c4fd8e9c18cc12f

Trott · 2018-07-05T22:42:59Z

Hi, @zokker13! Welcome and thanks for the PR! I have two requests for the documentation you've added:

Don't use **NOTE**:. If there's something important to say, just say it.
Don't use you in the docs.

These (well, the second one) are noted in doc/STYLE_GUIDE.md.

Trott · 2018-07-05T22:44:02Z

Feels notable enough that pinging @nodejs/tsc seems appropriate.

Also pinging @nodejs/documentation for any other comments on the docs.

mscdex · 2018-07-05T22:53:30Z

This sentence is confusing as Android is about as POSIX compliant as Linux is. It would be better to leave that part out and just say that the function is not available on Windows or Android.

I added Android support for this PR: a099642629b36e908c5114227c4fd8e9c18cc12f

mscdex · 2018-07-05T23:01:29Z

We should include return value information here since this function always returns a value.

Check. a099642629b36e908c5114227c4fd8e9c18cc12f

mscdex · 2018-07-05T23:08:21Z

Is it not possible to somehow bridge nice() and Windows' priority API via libuv?

Also, from what I'm reading, Android does support nice() (which is currently(?) just a wrapper around their own setpriority()).

Fishrock123 · 2018-07-06T00:03:38Z

Does this work on non-linuxies? I also wonder if the underlying impl isn’t better suited for libuv.

zokker13 · 2018-07-06T05:00:44Z

@mscdex Hey,
wrapping nice for Windows seems a bit odd to me. In Windows, we have those 6'ish different process priorities we might set. It doesn't really compare to the way nice() works since we merely add increment steps there. But I'll check out libuv and see how they solve similar issues later this day.
Can't really say anything about Android though. Could you share your find to the docs?

@Fishrock123 Yes, it should work for various POSIX compliant OS'. There are subtle differences between them though. But those differences are limited to the range of the niceness. So perhaps a reference to the min/max niceness would be fitting and convenient to the user (or at least a documentation).

gireeshpunathil · 2018-07-06T05:01:27Z

[ I am just thinking aload here, please don't consider as a negative remark on the PR. ]

are there any benchmarks that provide insights on scheduling behavior and its implications on the workload with default priority? I believe most of the UNIX schedulers have special optimizations for I/O workload, so wondering whether manual modification will influence the OS logic and thereby the overall performance positively or negatively.

Also wondering about the characteristics of stream velocity / backpressure etc. under a cluster, child process, IPC toppologies when processes are running with manually preset scheduling priorities.

If we don't have these info or cannot determine, it would be worthwhile considering writing caveats in the doc (or else we can get unwanted issues that complain about not getting desired througput after altering the nice etc.) on these lines: the overall efficiency of Node programs when catering to highly interactive workloads depend on peer end points resident in the host or otherwise. So altering the nice value of the process should be performed carefully, only after running your performance benchmark under full load and realizing any desired improvements.

mscdex · 2018-07-06T06:42:10Z

Hey, wrapping nice for Windows seems a bit odd to me. In Windows, we have those 6'ish different process priorities we might set. It doesn't really compare to the way nice() works since we merely add increment steps there.

I imagine someone at some point will want the Windows equivalent in core because process.nice() exists for non-Windows. If we can find a way to support it from the get-go, that would be better. Maybe we don't call it nice() if we support Windows with the same function. Or perhaps we support it in some other way, via a namespace or something else.

Can't really say anything about Android though. Could you share your find to the docs?

@zokker13 https://android.googlesource.com/platform/bionic.git/+/master/libc/upstream-netbsd/lib/libc/gen/nice.c

addaleax · 2018-07-06T08:56:06Z

@gireeshpunathil I don’t necessarily think it’s our job to talk about these things in the docs for nice(). I don’t know if you have increasing or decreasing niceness in mind, but the former will be more common (simply because it doesn’t require privileges), and you generally only do that if you don’t really care about performance all that much.

gireeshpunathil · 2018-07-06T08:59:42Z

ok, sure!

mscdex · 2018-07-07T00:28:52Z

I don't think having this is necessary, especially since we already document Windows is not supported below this example.

mscdex · 2018-07-07T00:32:52Z

Perhaps change this so the platform check is only run once when this file is evaluated, since the process platform will not change during runtime. Example:
if(process.platform==='win32'){process.nice=functionnice(inc){thrownewERR_METHOD_NOT_IMPLEMENTED('nice()');};}else{process.nice=functionnice(inc){validateInc(inc);return_nice(inc);};}
or something similar

mscdex · 2018-07-07T00:34:55Z

This is no longer valid since we're now throwing.
However, I feel like we should just call common.skip() with an appropriate message to make it obvious that we're not testing the real implementation.

mscdex · 2018-07-07T00:36:33Z

How about also testing that passing '0' or undefined results in a finite integer being returned?

Also, we might want to test that something like process.nice(1) works as expected.

zokker13 · 2018-07-07T07:55:21Z

Mh.. okay guys, I thought about having a more neutral function (say getpriority/setpritority) in favor of the nice function. The reason is that it doesn't really have a future on Windows and when we later add a functionality to set the priority cross-platform, nobody would use the posix function anyway.
That way the API is more clear and fewer obstacles are placed.

So I'd like to close this for now, go visit the libuv fellows and come back to implement a neutral call here.

addaleax · 2018-07-07T11:01:40Z

@zokker13 I think this PR is still in pretty good shape (I personally disagree about being inconsistent with other functions re: Windows, but, oh well). Can we mark this PR as in progress just so it doesn’t get merged, you try to work up something for libuv, and if that doesn’t pan out, we can continue where we left here?

The nice function allows us to fine-tune the process to meet desired scheduling behavior. If our process needs less schedule time because it is a long-running one, we can increase the nice value and cause the scheduler to select the process not so often.

zokker13 · 2018-07-07T15:07:37Z

@addaleax Ya, that sounds fine, thanks. I think the discussion alone is a good reason to go that route. Rebased just to be sure.

Perhaps @cjihrig would be so kind to guide me to the correct libuv location? The node structure is more familiar to me but libuv seems a bit mixed.

cjihrig · 2018-07-07T16:53:52Z

would be so kind to guide me to the correct libuv location?

You'd probably want to target https://github.com/libuv/libuv/blob/v1.x/src/unix/core.c and https://github.com/libuv/libuv/blob/v1.x/src/win/util.c.

Refs: nodejs/node#21675 PR-URL: libuv#1945 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

Refs: nodejs/node#21675 PR-URL: #1945 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

Notable changes: - Restores compatibility with the old IPC protocol. - Adds uv_open_osfhandle(). - Adds uv_os_{get,set}priority(). PR-URL: nodejs#22365Fixes: nodejs#21671Fixes: nodejs#15433 Refs: nodejs#21675 Refs: nodejs/node-addon-api#304 Refs: nodejs/abi-stable-node#318 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Notable changes: - Restores compatibility with the old IPC protocol. - Adds uv_open_osfhandle(). - Adds uv_os_{get,set}priority(). PR-URL: #22365Fixes: #21671Fixes: #15433 Refs: #21675 Refs: nodejs/node-addon-api#304 Refs: nodejs/abi-stable-node#318 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Fishrock123 · 2018-09-21T15:14:43Z

Superseded by the now-merged #22407

Notable changes: - Restores compatibility with the old IPC protocol. - Adds uv_open_osfhandle(). - Adds uv_os_{get,set}priority(). PR-URL: nodejs#22365Fixes: nodejs#21671Fixes: nodejs#15433 Refs: nodejs#21675 Refs: nodejs/node-addon-api#304 Refs: nodejs/abi-stable-node#318 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Notable changes: - Restores compatibility with the old IPC protocol. - Adds uv_open_osfhandle(). - Adds uv_os_{get,set}priority(). Backport-PR-URL: #24103 PR-URL: #22365Fixes: #21671Fixes: #15433 Refs: #21675 Refs: nodejs/node-addon-api#304 Refs: nodejs/abi-stable-node#318 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

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 Jul 5, 2018

addaleax reviewed Jul 5, 2018
View reviewed changes

devsnek reviewed Jul 5, 2018
View reviewed changes

Comment threaddoc/api/process.md Outdated
This comment was marked as resolved.
Sign in to view

devsnek approved these changes Jul 5, 2018
View reviewed changes

zokker13 force-pushed the add_nice_function branch from 84d744e to 0b5707bCompare July 5, 2018 21:52

addaleax approved these changes Jul 5, 2018
View reviewed changes

devsnek reviewed Jul 5, 2018
View reviewed changes

Trott reviewed Jul 5, 2018
View reviewed changes

mscdex reviewed Jul 5, 2018
View reviewed changes

gireeshpunathil approved these changes Jul 6, 2018
View reviewed changes

mscdex reviewed Jul 7, 2018
View reviewed changes

zokker13 added 7 commits July 7, 2018 17:03

process: add nice() function
c5aaa5a
The nice function allows us to fine-tune the process to meet desired scheduling behavior. If our process needs less schedule time because it is a long-running one, we can increase the nice value and cause the scheduler to select the process not so often.

fixup: follow review comments by addaleax
a9f80d4

fixup: follow review comments by devsnek
cc92503

fixup: follow review comments by various folk
bf86dcc

fixup: follow review comment by devsnek
e1388f3

fixup: throw not implemented on win32
01b0643

fixup: implement doc improvements by vsemozhetbyt
aadb65b

zokker13 force-pushed the add_nice_function branch from bd1e086 to aadb65bCompare July 7, 2018 15:03

addaleax added the wip Issues and PRs that are still a work in progress. label Jul 8, 2018

richardlau mentioned this pull request Aug 13, 2018
unix,win: add uv_os_{get,set}priority() libuv/libuv#1945
Merged

cjihrig added a commit to cjihrig/libuv that referenced this pull request Aug 15, 2018
unix,win: add uv_os_{get,set}priority()
81c7742
Refs: nodejs/node#21675 PR-URL: libuv#1945 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

cjihrig added a commit to libuv/libuv that referenced this pull request Aug 15, 2018
unix,win: add uv_os_{get,set}priority()
e57e071
Refs: nodejs/node#21675 PR-URL: #1945 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

cjihrig mentioned this pull request Aug 17, 2018
deps: upgrade to libuv 1.23.0 #22365
Merged

Fishrock123 closed this Sep 21, 2018

Uh oh!

Uh oh!

Conversation

zokker13 commented Jul 5, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jul 5, 2018

Uh oh!

This comment was marked as resolved.

Uh oh!

zokker13 commented Jul 5, 2018

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

devsnekJul 5, 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!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jul 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdexJul 5, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Jul 5, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Jul 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Jul 5, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fishrock123 commented Jul 6, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zokker13 commented Jul 6, 2018

Uh oh!

gireeshpunathil commented Jul 6, 2018

Uh oh!

mscdex commented Jul 6, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

zokker13 commented Jul 5, 2018•
edited
Loading

devsnekJul 5, 2018•
edited
Loading

mscdexJul 5, 2018•
edited
Loading

Trott commented Jul 5, 2018•
edited
Loading

mscdex commented Jul 5, 2018•
edited
Loading

Fishrock123 commented Jul 6, 2018•
edited
Loading

mscdex commented Jul 6, 2018•
edited
Loading

mscdexJul 7, 2018•
edited
Loading

mscdexJul 7, 2018•
edited
Loading