test: increase change_in_bytes for napi_adjust_external_memory test#57351
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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 added needs-ci 
| int64_tadjustedValue; | ||
| NODE_API_CALL(env, napi_adjust_external_memory(env, 1, &adjustedValue)); | ||
| // Some JavaScript engines may ignore small increments passed to napi_adjust_external_memory |
There was a problem hiding this comment.
Can you be more specific? What is the constraint you're seeing?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
IMO, the node-api adapter for WebKit should normalize small values if this is a WebKit constraint.
mhdawson left a comment
mhdawson left a comment There was a problem hiding this comment.
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.
Refs: nodejs#57351 - based on recent request to update one of the tests Signed-off-by: Michael Dawson <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 🚀 New features to boost your workflow:
|
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. |
mhdawson dismissed their stale reviewwant to wait until we finish discussion on doc update.
Refs: #57351 - based on recent request to update one of the tests Signed-off-by: Michael Dawson <[email protected]> PR-URL: #57463 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]>
Refs: #57351 - based on recent request to update one of the tests Signed-off-by: Michael Dawson <[email protected]> PR-URL: #57463 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]>
190n commented Mar 28, 2025
Given the new docs from #57463:
I think this tests change is no longer desired, right? |
mhdawson commented Mar 31, 2025
@190n correct I think the implementation should be updated to reflect the doc update and then the test should pass. |
github-project-automationbot moved this from Need Triage to Done in Node-API Team Project
Refs: nodejs#57351 - based on recent request to update one of the tests Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#57463 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]>
Refs: nodejs#57351 - based on recent request to update one of the tests Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#57463 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]>
Refs: #57351 - based on recent request to update one of the tests Signed-off-by: Michael Dawson <[email protected]> PR-URL: #57463 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]>
Refs: #57351 - based on recent request to update one of the tests Signed-off-by: Michael Dawson <[email protected]> PR-URL: #57463 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]>
Refs: #57351 - based on recent request to update one of the tests Signed-off-by: Michael Dawson <[email protected]> PR-URL: #57463 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]>
Refs: #57351 - based on recent request to update one of the tests Signed-off-by: Michael Dawson <[email protected]> PR-URL: #57463 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]>
Refs: #57351 - based on recent request to update one of the tests Signed-off-by: Michael Dawson <[email protected]> PR-URL: #57463 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]>
Refs: #57351 - based on recent request to update one of the tests Signed-off-by: Michael Dawson <[email protected]> PR-URL: #57463 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]>
Refs: #57351 - based on recent request to update one of the tests Signed-off-by: Michael Dawson <[email protected]> PR-URL: #57463 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]>
Refs: #57351 - based on recent request to update one of the tests Signed-off-by: Michael Dawson <[email protected]> PR-URL: #57463 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]>
Refs: #57351 - based on recent request to update one of the tests Signed-off-by: Michael Dawson <[email protected]> PR-URL: #57463 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]>
5 participants
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.