Skip to content

Conversation

theanarkh
Copy link
Contributor

@theanarkh theanarkh commented Jul 28, 2025

  1. constrained_memory maybe greater than uv_get_total_memory so we should take the minimum value.
    And uv_get_total_memory maybe 0.

  2. We should always report constrained memory in the report even though it is zero.

cc @Asaf-Federman

  • 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/config

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. config Issues or PRs related to the config subsystem needs-ci PRs that need a full CI run. report Issues and PRs related to process.report. labels Jul 28, 2025
@Asaf-Federman
Copy link
Contributor

Asaf-Federman commented Jul 28, 2025

AFAIK, constrained memory can be greater than the total physical memory due to swaps/overcommitting (which usually are set explicitly).
Any reason for taking the minimum between the virtual memory and the physical memory?

Copy link

codecov bot commented Jul 28, 2025

Codecov Report

❌ Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.04%. Comparing base (04c5a18) to head (e653171).
⚠️ Report is 58 commits behind head on main.

Files with missing lines Patch % Lines
src/node_options.cc 0.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #59260   +/-   ##
=======================================
  Coverage   90.04%   90.04%           
=======================================
  Files         648      648           
  Lines      191200   191201    +1     
  Branches    37472    37473    +1     
=======================================
+ Hits       172160   172170   +10     
+ Misses      11665    11625   -40     
- Partials     7375     7406   +31     
Files with missing lines Coverage Δ
src/node_report.cc 93.36% <100.00%> (+0.14%) ⬆️
src/node_options.cc 84.40% <0.00%> (-0.29%) ⬇️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@himself65

This comment was marked as off-topic.

@Asaf-Federman
Copy link
Contributor

related to #59227

This does not seem related

@himself65
Copy link
Member

yeah, I misunderstood

@theanarkh
Copy link
Contributor Author

AFAIK, constrained memory can be greater than the total physical memory due to swaps/overcommitting (which usually are set explicitly). Any reason for taking the minimum between the virtual memory and the physical memory?

It is consistent with this. But i think you are right that the virtual memory can greater than physical memory. @bnoordhuis Hi ! could you please help take a look at this ? Thanks !

@@ -135,9 +135,13 @@ void PerIsolateOptions::HandleMaxOldSpaceSizePercentage(
// Use uint64_t for the result to prevent data loss on 32-bit systems.
uint64_t available_memory =
(constrained_memory > 0 && constrained_memory != UINT64_MAX)
? constrained_memory
? std::min(total_memory, constrained_memory)
Copy link
Contributor

Choose a reason for hiding this comment

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

As you correctly noted, uv_get_total_memory returns 0 when its value is unknown. This might become problematic if uv_get_constrained_memory is defined but uv_get_total_memory is not, as the min function would incorrectly return 0 despite a valid constraint being available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It seem we should remove std::min in SetIsolateCreateParamsForNode.

@Asaf-Federman
Copy link
Contributor

AFAIK, constrained memory can be greater than the total physical memory due to swaps/overcommitting (which usually are set explicitly). Any reason for taking the minimum between the virtual memory and the physical memory?

In summary, uv_get_constrained_memory reports the memory limit directly from cgroups—the Linux feature that orchestrators like Kubernetes and runtimes like Docker use to isolate resources.

The function correctly reflects the container's configured limit, not the host's physical RAM. This behavior is essential for advanced techniques like memory overcommitment, which carries a significant risk of performance degradation if mismanaged.

Given these trade-offs, I'm +0 on changing this feature.

@theanarkh theanarkh closed this Aug 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. config Issues or PRs related to the config subsystem needs-ci PRs that need a full CI run. report Issues and PRs related to process.report.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants