Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src,lib: add constrainedMemory API for process #46218

Merged
merged 1 commit into from Jan 25, 2023
Merged

src,lib: add constrainedMemory API for process #46218

merged 1 commit into from Jan 25, 2023
+40 −0

Conversation

theanarkh
Copy link
Contributor

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
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot 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 theanarkh force-pushed the add_constrainedMemory_for_process branch from 7b17275 to 5540f21 Compare January 15, 2023 05:01
ajafff
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 theanarkh force-pushed the add_constrainedMemory_for_process branch from 5540f21 to 3fd6390 Compare January 15, 2023 11:42
@RaisinTen
Copy link
Contributor

Could also add some docs?

@theanarkh
Copy link
Contributor Author

Could also add some docs?

Oh, sorry I forgot, fixed.

@theanarkh theanarkh force-pushed the add_constrainedMemory_for_process branch 2 times, most recently from fd857a6 to 262ae5e Compare January 16, 2023 13:56
jasnell
jasnell reviewed Jan 17, 2023
View reviewed changes
doc/api/process.md Outdated
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description was copied from the Libuv documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bnoordhuis
bnoordhuis reviewed Jan 18, 2023
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two suggestions:

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

  2. 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
Copy link
Contributor Author

theanarkh commented Jan 19, 2023 •
edited

Two suggestions:

  1. Maybe add an Experimental marker to the docs. Libuv's implementation changed in the not too distant past and it may change again
  2. 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 !

tniessen
tniessen reviewed Jan 19, 2023
View reviewed changes
doc/api/process.md Outdated Show resolved Hide resolved
@anonrig
Copy link
Member

anonrig commented Jan 19, 2023

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.

@theanarkh theanarkh force-pushed the add_constrainedMemory_for_process branch from 262ae5e to c5d496f Compare January 20, 2023 02:28
RaisinTen
RaisinTen reviewed Jan 20, 2023
View reviewed changes
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
@theanarkh theanarkh force-pushed the add_constrainedMemory_for_process branch 2 times, most recently from 1ac7401 to fca8b25 Compare January 20, 2023 07:18
@theanarkh
Copy link
Contributor Author

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

tniessen
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 theanarkh force-pushed the add_constrainedMemory_for_process branch from fca8b25 to 798d817 Compare January 20, 2023 11:27
@tniessen tniessen added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 20, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Jan 23, 2023
Open
Open
@theanarkh 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
@theanarkh
Copy link
Contributor Author

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

@github-actions github-actions bot mentioned this pull request Jan 25, 2023
Open
12 tasks
@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 25, 2023
@anonrig
Copy link
Member

anonrig commented Jan 25, 2023 •
edited

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

@nodejs-github-bot 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 nodejs-github-bot merged commit 73c0564 into nodejs:main Jan 25, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 73c0564

ruyadorno pushed a commit that referenced this pull request Feb 1, 2023
@theanarkh @ruyadorno
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 ruyadorno mentioned this pull request Feb 1, 2023
Merged
@ruyadorno ruyadorno added the notable-change PRs with changes that should be highlighted in changelogs. label Feb 1, 2023
ruyadorno added a commit that referenced this pull request Feb 1, 2023
@ruyadorno
2023-02-02, Version 19.6.0 (Current)
3ecd96f 
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
ruyadorno added a commit that referenced this pull request Feb 2, 2023
@ruyadorno
2023-02-02, Version 19.6.0 (Current)
1579ff4 
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
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
@theanarkh @juanarbol
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 added a commit that referenced this pull request Mar 3, 2023
@juanarbol
2023-03-07, Version 18.15.0 'Hydrogen' (LTS), @juanarbol
1fd3ca2 
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
@juanarbol juanarbol mentioned this pull request Mar 3, 2023
Merged
juanarbol added a commit that referenced this pull request Mar 3, 2023
@juanarbol
2023-03-07, Version 18.15.0 'Hydrogen' (LTS), @juanarbol
6b8ecaa 
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
juanarbol added a commit that referenced this pull request Mar 3, 2023
@juanarbol
2023-03-07, Version 18.15.0 'Hydrogen' (LTS)
6859857 
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
juanarbol added a commit that referenced this pull request Mar 3, 2023
@juanarbol
2023-03-07, Version 18.15.0 'Hydrogen' (LTS)
6110438 
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
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
@theanarkh @juanarbol
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>
juanarbol added a commit that referenced this pull request Mar 5, 2023
@juanarbol
2023-03-07, Version 18.15.0 'Hydrogen' (LTS)
c1d6a15 
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
juanarbol added a commit that referenced this pull request Mar 5, 2023
@juanarbol
2023-03-07, Version 18.15.0 'Hydrogen' (LTS)
fceac82 
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
juanarbol added a commit that referenced this pull request Mar 5, 2023
@juanarbol
2023-03-07, Version 18.15.0 'Hydrogen' (LTS)
a94966f 
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
juanarbol added a commit that referenced this pull request Mar 7, 2023
@juanarbol
2023-03-07, Version 18.15.0 'Hydrogen' (LTS)
3b0c047 
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
@richardlau richardlau mentioned this pull request Dec 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments

@jasnell jasnell jasnell left review comments

@ajafff ajafff ajafff left review comments

@anonrig anonrig anonrig approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

@tniessen tniessen Awaiting requested review from tniessen

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. process Issues and PRs related to the process subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@theanarkh @nodejs-github-bot @RaisinTen @anonrig @bnoordhuis @jasnell @tniessen @ajafff @ruyadorno