process: add nice() function #21675

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

doc/api/process.md

@@ -1642,6 +1642,40 @@ This function is only available on POSIX platforms (i.e. not Windows or
 Android).
 This feature is not available in [`Worker`][] threads.

+## process.nice(inc)
+<!-- YAML
+added: v10.?.?


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

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

src/node_process.cc

+  // ..only check type if argument is int32
+  if (!args[0]->IsUndefined()) {
+    CHECK(args[0]->IsInt32());
+    inc = args[0]->Int32Value();


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

lib/internal/process/main_thread_only.js

@@ -98,6 +104,18 @@ function setupPosixMethods(_initgroups, _setegid, _seteuid,
    }
  };

+  function validateInc(inc) {
+    const tInc = typeof inc;


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

lib/internal/process/main_thread_only.js

+      validateInt32(inc, 'nice');
+      return _nice(inc);
+    } else if (tInc === 'undefined') {
+      return _nice(inc);


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.

doc/api/process.md

+The higher the nice value of the process, the nicer the process and the less
+time is given by the scheduler.
+
+You can get the current nice value by passing 0 or nothing to the method.


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

lib/internal/process/main_thread_only.js

-      return _nice(inc);
-    } else if (tInc === 'undefined') {
-      return _nice(inc);
+    } else if (typeof inc === 'undefined') {


you can just combine this with the below

function validateInc(inc) { if (typeof inc === 'number') { validateInt32(inc, 'nice'); } else if (inc !== undefined) { throw new ERR_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

doc/api/process.md

+You can get the current nice value by passing 0 or nothing to the method.
+
+Unless you have super user permissions, you can only increase your nice value.
+Though, it is also possible to increase the priority if a limit (RLIMIT_NICE)


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

doc/api/process.md

+Though, it is also possible to increase the priority if a limit (RLIMIT_NICE)
+is set. (See getrlimit(2).)
+
+**NOTE**: Once you incremented your niceness, you can no longer reduce it!


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

doc/api/process.md

+```
+
+This function is only available on POSIX platforms (i.e. not Windows or
+Android).


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

doc/api/process.md

+added: REPLACEME
+-->
+
+* `inc` {integer} The new nice value for the process.


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

doc/api/process.md

+the specified permissions are set). (See getrlimit(2).)
+
+```js
+if (process.platform !== 'win32') {


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

lib/internal/process/main_thread_only.js

  if (_setgid !== undefined) {
    setupPosixMethods(_initgroups, _setegid, _seteuid,
                      _setgid, _setuid, _setgroups);
  }

+  process.nice = function nice(inc) {
+    if (process.platform === 'win32') {


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 = function nice(inc) { throw new ERR_METHOD_NOT_IMPLEMENTED('nice()'); }; } else { process.nice = function nice(inc) { validateInc(inc); return _nice(inc); }; }

or something similar

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

test/parallel/test-process-nice.js

+const assert = require('assert');
+
+if (common.isWindows || !common.isMainThread) {
+  assert.strictEqual(process.nice, undefined);


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

test/parallel/test-process-nice.js

+  }
+);
+
+[null, false, true, {}, [], () => {}, 'text'].forEach((val) => {


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#22365 Fixes: nodejs#21671 Fixes: 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: #22365 Fixes: #21671 Fixes: #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#22365 Fixes: nodejs#21671 Fixes: 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: #22365 Fixes: #21671 Fixes: #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

doc/api/process.md Outdated

The higher the nice value of the process, the nicer the process and the less

time is given by the scheduler.

You can get the current nice value by passing 0 or nothing to the method.

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 0b5707b Compare 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 aadb65b Compare 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

process: add nice() function #21675

process: add nice() function #21675

Conversation

zokker13 commented Jul 5, 2018 • edited

Checklist

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jul 5, 2018

This comment was marked as resolved.

zokker13 commented Jul 5, 2018

addaleax left a comment

Choose a reason for hiding this comment

devsnek Jul 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jul 5, 2018

Choose a reason for hiding this comment

mscdex Jul 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Jul 5, 2018 • edited

Trott commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Jul 5, 2018 • edited

Fishrock123 commented Jul 6, 2018 • edited

zokker13 commented Jul 6, 2018

gireeshpunathil commented Jul 6, 2018

mscdex commented Jul 6, 2018 • edited

addaleax commented Jul 6, 2018

gireeshpunathil commented Jul 6, 2018

Choose a reason for hiding this comment

mscdex Jul 7, 2018 • edited

Choose a reason for hiding this comment

mscdex Jul 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zokker13 commented Jul 7, 2018

addaleax commented Jul 7, 2018

zokker13 commented Jul 7, 2018

cjihrig commented Jul 7, 2018

Fishrock123 commented Sep 21, 2018

zokker13 commented Jul 5, 2018 •

edited

devsnek Jul 5, 2018 •

edited

mscdex Jul 5, 2018 •

edited

Trott commented Jul 5, 2018 •

edited

mscdex commented Jul 5, 2018 •

edited

Fishrock123 commented Jul 6, 2018 •

edited

mscdex commented Jul 6, 2018 •

edited

mscdex Jul 7, 2018 •

edited

mscdex Jul 7, 2018 •

edited