src,lib: add constrainedMemory API for process by theanarkh · Pull Request #46218 · nodejs/node

theanarkh · 2023-01-15T04:59:00Z

add constrainedMemory API for process.
cc @anonrig

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

nodejs-github-bot · 2023-01-15T04:59:04Z

Review requested:

@nodejs/startup

lib/internal/process/per_thread.js

RaisinTen · 2023-01-16T05:45:58Z

Could also add some docs?

theanarkh · 2023-01-16T13:47:21Z

Could also add some docs?

Oh, sorry I forgot, fixed.

jasnell · 2023-01-17T15:15:09Z

doc/api/process.md

+is unknown, `0` is returned.
+
+It is not unusual for this value to be less than or greater than `os.totalmem()`.
+This function currently only returns a non-zero value on Linux, based on cgroups


This sentence is worded a bit confusingly. What is the value on Windows?

The description was copied from the Libuv documentation.

That may be true but it's still confusing as is. Not sure I have a better suggestion tho

doc/api/process.md

test/sequential/test-process-constrained-memory.js

bnoordhuis

Two suggestions:

Maybe add an Experimental marker to the docs. Libuv's implementation changed in the not too distant past and it may change again
Implementing it as a fast API is probably overkill. The limiting factor is the amount of file parsing libuv has to do. And also, you're probably not going to call it 1,000x/sec.

theanarkh · 2023-01-19T05:25:29Z

Two suggestions:

Maybe add an Experimental marker to the docs. Libuv's implementation changed in the not too distant past and it may change again

Implementing it as a fast API is probably overkill. The limiting factor is the amount of file parsing libuv has to do. And also, you're probably not going to call it 1,000x/sec.

Thanks for suggestions, Hi @anonrig, would you mind taking a look at the second suggestion. Thanks !

doc/api/process.md

anonrig · 2023-01-19T18:05:14Z

Thanks for suggestions, Hi @anonrig, would you mind taking a look at the second suggestion. Thanks !

I agree that it is an overkill, but I don't see any downside of adding fast API. This might be important/valuable functionality which result in high usage (for example APMs). But if @bnoordhuis or anybody disagrees with it, I'm fine with removing it.

RaisinTen

I don't see any downside of adding fast API

From what I read in #46199, the fast API would only be triggered if the function is called in a loop with a high iteration count. Unless that's something we expect, the fast API would be a function with little to no usage.

doc/api/process.md

theanarkh · 2023-01-20T07:20:43Z

Thanks for all suggestions, i have removed the fast API code, please help review again.

doc/api/process.md

src/node_process_methods.cc

test/parallel/test-process-constrained-memory.js

nodejs-github-bot · 2023-01-22T04:36:02Z

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

nodejs-github-bot · 2023-01-22T08:50:59Z

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

theanarkh · 2023-01-24T08:18:20Z

@tniessen Hi, can help this PR again ? thanks .

anonrig · 2023-01-25T00:40:01Z

There weren't any objections, blockers and/or active conversations. We can merge it.

nodejs-github-bot · 2023-01-25T00:47:10Z

Landed in 73c0564

PR-URL: #46218 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

Notable changes: buffer: * (SEMVER-MINOR) add isAscii method (Yagiz Nizipli) #46046 deps: * upgrade npm to 9.4.0 (npm team) #46353 esm: * leverage loaders when resolving subsequent loaders (Maël Nison) #43772 fs: * (SEMVER-MINOR) add statfs() functions (Colin Ihrig) #46358 src,lib: * (SEMVER-MINOR) add constrainedMemory API for process (theanarkh) #46218 test_runner: * (SEMVER-MINOR) add reporters (Moshe Atlow) #45712 v8: * (SEMVER-MINOR) support gc profile (theanarkh) #46255 vm: * (SEMVER-MINOR) expose cachedDataRejected for vm.compileFunction (Anna Henningsen) #46320 PR-URL: #46455

PR-URL: #46218 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

Notable Changes buffer: * add isAscii method (Yagiz Nizipli) #46046 fs: * add statfs() functions (Colin Ihrig) #46358 src,lib: * add constrainedMemory API for process (theanarkh) #46218 v8: * support gc profile (theanarkh) #46255 vm: * expose cachedDataRejected for vm.compileFunction (Anna Henningsen) #46320 PR-URL: TDB

Notable Changes buffer: * add isAscii method (Yagiz Nizipli) #46046 fs: * add statfs() functions (Colin Ihrig) #46358 src,lib: * add constrainedMemory API for process (theanarkh) #46218 v8: * support gc profile (theanarkh) #46255 vm: * expose cachedDataRejected for vm.compileFunction (Anna Henningsen) #46320 PR-URL: #46920

Notable Changes: buffer: * add isAscii method (Yagiz Nizipli) #46046 fs: * add statfs() functions (Colin Ihrig) #46358 src,lib: * add constrainedMemory API for process (theanarkh) #46218 v8: * support gc profile (theanarkh) #46255 vm: * expose cachedDataRejected for vm.compileFunction (Anna Henningsen) #46320 PR-URL: #46920

Notable Changes: buffer: * (SEMVER-MINOR) add isAscii method (Yagiz Nizipli) #46046 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 fs: * (SEMVER-MINOR) add statfs() functions (Colin Ihrig) #46358 src,lib: * (SEMVER-MINOR) add constrainedMemory API for process (theanarkh) #46218 test_runner: * add initial code coverage support (Colin Ihrig) #46017 * (SEMVER-MINOR) add reporters (Moshe Atlow) #45712 v8: * (SEMVER-MINOR) support gc profile (theanarkh) #46255 vm: * (SEMVER-MINOR) expose cachedDataRejected for vm.compileFunction (Anna Henningsen) #46320 PR-URL: #46920

PR-URL: #46218 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

Notable changes: buffer: * (SEMVER-MINOR) add isAscii method (Yagiz Nizipli) #46046 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 fs: * (SEMVER-MINOR) add statfs() functions (Colin Ihrig) #46358 src,lib: * (SEMVER-MINOR) add constrainedMemory API for process (theanarkh) #46218 test_runner: * add initial code coverage support (Colin Ihrig) #46017 * (SEMVER-MINOR) add reporters (Moshe Atlow) #45712 v8: * (SEMVER-MINOR) support gc profile (theanarkh) #46255 vm: * (SEMVER-MINOR) expose cachedDataRejected for vm.compileFunction (Anna Henningsen) #46320 PR-URL: https://github.com/nodejs/node/pull/46920/commits

Notable changes: buffer: * (SEMVER-MINOR) add isAscii method (Yagiz Nizipli) #46046 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 fs: * (SEMVER-MINOR) add statfs() functions (Colin Ihrig) #46358 src,lib: * (SEMVER-MINOR) add constrainedMemory API for process (theanarkh) #46218 test_runner: * add initial code coverage support (Colin Ihrig) #46017 * (SEMVER-MINOR) add reporters (Moshe Atlow) #45712 v8: * (SEMVER-MINOR) support gc profile (theanarkh) #46255 vm: * (SEMVER-MINOR) expose cachedDataRejected for vm.compileFunction (Anna Henningsen) #46320 PR-URL: #46920

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Jan 15, 2023

theanarkh force-pushed the add_constrainedMemory_for_process branch from 7b17275 to 5540f21 Compare January 15, 2023 05:01

anonrig approved these changes Jan 15, 2023

View reviewed changes

ajafff reviewed Jan 15, 2023

View reviewed changes

lib/internal/process/per_thread.js Outdated Show resolved Hide resolved

lib/internal/process/per_thread.js Outdated Show resolved Hide resolved

theanarkh force-pushed the add_constrainedMemory_for_process branch from 5540f21 to 3fd6390 Compare January 15, 2023 11:42

theanarkh force-pushed the add_constrainedMemory_for_process branch 2 times, most recently from fd857a6 to 262ae5e Compare January 16, 2023 13:56

jasnell reviewed Jan 17, 2023

View reviewed changes

doc/api/process.md Show resolved Hide resolved

jasnell reviewed Jan 17, 2023

View reviewed changes

test/sequential/test-process-constrained-memory.js Outdated Show resolved Hide resolved

bnoordhuis reviewed Jan 18, 2023

View reviewed changes

anonrig approved these changes Jan 19, 2023

View reviewed changes

tniessen reviewed Jan 19, 2023

View reviewed changes

doc/api/process.md Outdated Show resolved Hide resolved

theanarkh force-pushed the add_constrainedMemory_for_process branch from 262ae5e to c5d496f Compare January 20, 2023 02:28

RaisinTen reviewed Jan 20, 2023

View reviewed changes

doc/api/process.md Show resolved Hide resolved

theanarkh force-pushed the add_constrainedMemory_for_process branch 2 times, most recently from 1ac7401 to fca8b25 Compare January 20, 2023 07:18

RaisinTen approved these changes Jan 20, 2023

View reviewed changes

tniessen reviewed Jan 20, 2023

View reviewed changes

doc/api/process.md Outdated Show resolved Hide resolved

doc/api/process.md Outdated Show resolved Hide resolved

src/node_process_methods.cc Outdated Show resolved Hide resolved

theanarkh force-pushed the add_constrainedMemory_for_process branch from fca8b25 to 798d817 Compare January 20, 2023 11:27

tniessen reviewed Jan 20, 2023

View reviewed changes

test/parallel/test-process-constrained-memory.js Outdated Show resolved Hide resolved

tniessen added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 20, 2023

theanarkh requested a review from tniessen January 22, 2023 04:43

This was referenced Jan 23, 2023

CI Reliability 2023-01-23 nodejs/reliability#492

Open

CI Reliability 2023-01-24 nodejs/reliability#493

Open

theanarkh added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 24, 2023

github-actions bot mentioned this pull request Jan 25, 2023

CI Reliability 2023-01-25 nodejs/reliability#494

Open

12 tasks

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 25, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 25, 2023

nodejs-github-bot merged commit 73c0564 into nodejs:main Jan 25, 2023

ruyadorno pushed a commit that referenced this pull request Feb 1, 2023

src,lib: add constrainedMemory API for process

d52f600

PR-URL: #46218 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

ruyadorno mentioned this pull request Feb 1, 2023

v19.6.0 proposal #46455

Merged

ruyadorno added the notable-change PRs with changes that should be highlighted in changelogs. label Feb 1, 2023

juanarbol pushed a commit that referenced this pull request Mar 3, 2023

src,lib: add constrainedMemory API for process

3075588

PR-URL: #46218 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

juanarbol mentioned this pull request Mar 3, 2023

V18.15.0 proposal #46920

Merged

juanarbol pushed a commit that referenced this pull request Mar 5, 2023

src,lib: add constrainedMemory API for process

f09b838

PR-URL: #46218 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

richardlau mentioned this pull request Dec 9, 2023

lib, src: add maxmem prop in os module #51102

Closed

Uh oh!

Conversation

theanarkh commented Jan 15, 2023

Uh oh!

nodejs-github-bot commented Jan 15, 2023

Uh oh!

Uh oh!

Uh oh!

RaisinTen commented Jan 16, 2023

Uh oh!

theanarkh commented Jan 16, 2023

Uh oh!

jasnell Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

theanarkh Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

jasnell Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

theanarkh commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

anonrig commented Jan 19, 2023

Uh oh!

RaisinTen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theanarkh commented Jan 20, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 22, 2023

Uh oh!

nodejs-github-bot commented Jan 22, 2023

Uh oh!

theanarkh commented Jan 24, 2023

Uh oh!

anonrig commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

theanarkh commented Jan 19, 2023 •

edited

Loading

anonrig commented Jan 25, 2023 •

edited

Loading