Skip to content

test: increase change_in_bytes for napi_adjust_external_memory test #57351

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.

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

Closed
wants to merge 1 commit into from

Conversation

190n
Copy link

@190n 190n commented Mar 6, 2025

JavaScriptCore ignores small deltas in its interface for reporting external memory:

https://github.com/WebKit/WebKit/blob/8bbba2aa777cf326e24a4e5c63b1f51bd52315ff/Source/JavaScriptCore/heap/HeapInlines.h#L213-L229

More broadly, it seem reasonable for any other JS engine to do the same. Passing a large change ensures that the change is actually applied and the new adjusted value will be greater than zero (otherwise, the test relies on some external memory previously being reported as allocated). In turn, this makes the test more applicable to non-V8 implementations of Node-API.

JavaScriptCore ignores small deltas in its interface for reporting
external memory:

https://github.com/WebKit/WebKit/blob/8bbba2aa777cf326e24a4e5c63b1f51bd52315ff/Source/JavaScriptCore/heap/HeapInlines.h#L213-L229

More broadly, it seem reasonable for any other JS engine to do the same.
Passing a large change ensures that the change is actually applied and
the new adjusted value will be greater than zero (otherwise, the test
relies on some external memory previously being reported as allocated).
In turn, this makes the test more applicable to non-V8 implementations
of Node-API.
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Mar 6, 2025
@@ -215,7 +215,8 @@ static napi_value testAdjustExternalMemory(napi_env env, napi_callback_info info
napi_value result;
int64_t adjustedValue;

NODE_API_CALL(env, napi_adjust_external_memory(env, 1, &adjustedValue));
// Some JavaScript engines may ignore small increments passed to napi_adjust_external_memory
Copy link
Member

Choose a reason for hiding this comment

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

Can you be more specific? What is the constraint you're seeing?

Copy link
Author

Choose a reason for hiding this comment

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

For JSC, right now the cutoff is set to 256 bytes: https://github.com/WebKit/WebKit/blob/8bbba2aa777cf326e24a4e5c63b1f51bd52315ff/Source/JavaScriptCore/heap/Heap.h#L645

I realize another possible solution is that implementations with imprecise external memory tracking could treat change_in_bytes as a floor on the return value -- basically, pretending that at least the currently requested change has had an effect. On Node's side this would basically be a docs change instead of a test change. At a glance from the PR #14310 where this was originally added, it seems like the question of what to do with an engine that cannot track external memory at all was brought up but not resolved, and it doesn't look like the possibility of memory tracking existing but with a minimum was brought up. The documentation only really describes this function's effect in V8.

I'd be happy with either approach.

Copy link
Member

Choose a reason for hiding this comment

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

IMO, the node-api adapter for WebKit should normalize small values if this is a WebKit constraint.

mhdawson
mhdawson previously approved these changes Mar 14, 2025
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM as the function is just providing additional info to the runtime which may or may not use. I'll submit a PR to add some additional info to the documentation to clarify that.

mhdawson added a commit to mhdawson/io.js that referenced this pull request Mar 14, 2025
Refs: nodejs#57351
- based on recent request to update one of the tests

Signed-off-by: Michael Dawson <midawson@redhat.com>
Copy link

codecov bot commented Mar 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.21%. Comparing base (a790901) to head (3a3bc8f).
Report is 145 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #57351      +/-   ##
==========================================
- Coverage   90.21%   90.21%   -0.01%     
==========================================
  Files         630      630              
  Lines      185304   185304              
  Branches    36266    36267       +1     
==========================================
- Hits       167171   167164       -7     
+ Misses      11084    11083       -1     
- Partials     7049     7057       +8     

see 28 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.

@legendecas
Copy link
Member

legendecas commented Mar 17, 2025

I think this should wait for #57463 (comment) to be resolved first. Add-ons may call the function with an arbitrary value of the size of allocated memory block, and there was no smallest limitation on the size.

@legendecas legendecas added the blocked PRs that are blocked by other issues or PRs. label Mar 17, 2025
@mhdawson mhdawson dismissed their stale review March 17, 2025 15:14

want to wait until we finish discussion on doc update.

nodejs-github-bot pushed a commit that referenced this pull request Mar 28, 2025
Refs: #57351
- based on recent request to update one of the tests

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #57463
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
aduh95 pushed a commit that referenced this pull request Mar 28, 2025
Refs: #57351
- based on recent request to update one of the tests

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #57463
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
@190n
Copy link
Author

190n commented Mar 28, 2025

Given the new docs from #57463:

  • [out] result: The adjusted value. This value should reflect the
    total amount of external memory with the given change_in_bytes included.

I think this tests change is no longer desired, right?

@mhdawson
Copy link
Member

@190n correct I think the implementation should be updated to reflect the doc update and then the test should pass.

@190n 190n closed this Mar 31, 2025
@github-project-automation github-project-automation bot moved this from Need Triage to Done in Node-API Team Project Mar 31, 2025
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Apr 8, 2025
Refs: nodejs#57351
- based on recent request to update one of the tests

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#57463
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
JonasBa pushed a commit to JonasBa/node that referenced this pull request Apr 11, 2025
Refs: nodejs#57351
- based on recent request to update one of the tests

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#57463
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
RafaelGSS pushed a commit that referenced this pull request Apr 14, 2025
Refs: #57351
- based on recent request to update one of the tests

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #57463
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
RafaelGSS pushed a commit that referenced this pull request Apr 14, 2025
Refs: #57351
- based on recent request to update one of the tests

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #57463
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
aduh95 pushed a commit that referenced this pull request Apr 14, 2025
Refs: #57351
- based on recent request to update one of the tests

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #57463
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
aduh95 pushed a commit that referenced this pull request Apr 14, 2025
Refs: #57351
- based on recent request to update one of the tests

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #57463
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
aduh95 pushed a commit that referenced this pull request Apr 15, 2025
Refs: #57351
- based on recent request to update one of the tests

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #57463
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
RafaelGSS pushed a commit that referenced this pull request Apr 16, 2025
Refs: #57351
- based on recent request to update one of the tests

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #57463
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants