n-api: adds function to adjust external memory #14310

chris--young · 2017-07-16T20:07:32Z

Added a simple wrapper around v8::Isolate::AdjustAmountOfExternalAllocatedMemory

One of the requests from @mhdawson's original issue was for this to act as a no-op on unsupported vms. I am a little unsure of the best way to handle that. As far as I can tell Chakra is the only unsupported vm we need to do account for and figure the easiest thing to do would be for me to open a second pr against node-charkracore where napi_adjust_external_memory() always returns -1 (or something along those lines). But I'm worried that will cause a conflict when merging the two repos.

Fixes: #13928

Checklist

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

Affected core subsystem(s)

n-api

addaleax

Two comments, otherwise LGTM – thanks!

addaleax · 2017-07-16T21:06:39Z

doc/api/n-api.md

@@ -3155,6 +3155,21 @@ support it:
 * If the function is not available, provide an alternate implementation
  that does not use the function.

+### napi_adjust_external_memory
+<!-- YAML
+added: v8.0.0


Can you use added: REPLACEME here?

addaleax · 2017-07-16T21:07:25Z

test/addons-napi/test_general/test.js

@@ -58,3 +58,6 @@ test_general.wrap(x, 25);
 assert.throws(function() {
  test_general.wrap(x, 'Blah');
 }, Error);
+
+// test napi_adjust_external_memory
+assert.ok(typeof test_general.testAdjustExternalMemory() === 'number');


Can you use assert.strictEqual for this kind of check?

mhdawson · 2017-07-18T00:09:10Z

doc/api/n-api.md

+-->
+```C
+NAPI_EXTERN int64_t napi_adjust_external_memory(napi_env env,
+                                         int64_t change_in_bytes);


I think the parameters should be aligned here, that probably means the first one will have to be on a new line. I'm surprised the linter did not complain about this.

We don't run the C++ linter against Markdown files yet.

mhdawson · 2017-07-18T00:10:20Z

doc/api/n-api.md

@@ -3170,6 +3185,7 @@ support it:
 [Working with JavaScript Values]: #n_api_working_with_javascript_values
 [Working with JavaScript Values - Abstract Operations]: #n_api_working_with_javascript_values_abstract_operations

+[`napi_adjust_external_memory`]: #n_api_napi_adjust_external_memory


I don't think we need this unless there is a reference to it.

mhdawson · 2017-07-18T00:11:21Z

src/node_api.h

@@ -512,6 +512,10 @@ NAPI_EXTERN napi_status napi_cancel_async_work(napi_env env,
 // version management
 NAPI_EXTERN napi_status napi_get_version(napi_env env, uint32_t* result);

+// Garbage collection


Not sure about the comment. Does adjusting the external memory trigger a gc ?

No, I don't think it triggers a gc but it is related to gc and I wanted to avoid any confusion with the "version management" comment above. I can remove this line if you think that would be best.

It controls the operations of the GC, so I think the heading is appropriate.

Needs more detail though. Many people will wonder the same thing as @mhdawson when it's just one of many input signals that influence the frequency of GC runs.

mhdawson · 2017-07-18T00:14:03Z

Thanks for picking this up. In terms of the Chakra implementation I think we (the N-API team) agreed to let the Microsoft team figure out how to implement new functions like this. Left a few comments.

TimothyGu

LGTM besides the doc nit.

TimothyGu · 2017-07-18T01:15:25Z

doc/api/n-api.md

@@ -3155,6 +3155,21 @@ support it:
 * If the function is not available, provide an alternate implementation
  that does not use the function.

+### napi_adjust_external_memory


I think you would need a "Garbage Collection" heading here as well, so as to not categorize it under "Version Management".

cjihrig · 2017-07-18T12:08:40Z

src/node_api.h

@@ -512,6 +512,10 @@ NAPI_EXTERN napi_status napi_cancel_async_work(napi_env env,
 // version management
 NAPI_EXTERN napi_status napi_get_version(napi_env env, uint32_t* result);

+// Garbage collection
+NAPI_EXTERN int64_t napi_adjust_external_memory(napi_env env,


I think napi_status napi_adjust_external_memory(napi_env env, int64_t change_in_bytes, int64_t* result) would be more consistent with the rest of the API, since almost everything else returns a napi_status.

cjihrig · 2017-07-24T13:45:08Z

doc/api/n-api.md

+kept alive by JavaScript objects.
+- `[out] result`: The adjusted value
+
+Returns `napi_ok` if the API succeeded.


Seems like this is missing a description of what the method actually does.

Added a description below

cjihrig · 2017-07-24T13:48:50Z

test/addons-napi/test_general/test.js

@@ -58,3 +58,6 @@ test_general.wrap(x, 25);
 assert.throws(function() {
  test_general.wrap(x, 'Blah');
 }, Error);
+
+// test napi_adjust_external_memory
+assert.notStrictEqual(test_general.testAdjustExternalMemory(), -1);


Can you add some tests for invalid values.

mhdawson · 2017-08-17T22:49:59Z

@chris--young you still working on this ?

chris--young · 2017-08-17T23:05:15Z

@mhdawson yea ill wrap it up this weekend

digitalinfinity

LGTM (although agree with @cjihrig that we should document it more precisely). For the node-chakracore side of the house, we welcome PRs 😄 - if you ping me on the ChakraCore gitter channel, happy to help guide you to make that change.

mhdawson · 2017-08-18T14:14:16Z

@chris--young thanks for your help on this :)

chris--young · 2017-08-21T05:54:01Z

@cjihrig please review again

I looked into testing for invalid values, but it seems like there isn't much to be done there. V8 handles the change gracefully even if you exceed the external allocation limit or pass in 0. Any other bad values should be prevented by the compiler.

chris--young · 2017-08-21T06:01:44Z

@digitalinfinity i'd definitely like to help out on the node-chakracore side too. I'll follow up with you after my changes here have been fully approved.

mhdawson

LGTM

mhdawson · 2017-07-18T00:13:06Z

test/addons-napi/test_general/test.js

@@ -58,3 +58,6 @@ test_general.wrap(x, 25);
 assert.throws(function() {
  test_general.wrap(x, 'Blah');
 }, Error);
+
+// test napi_adjust_external_memory
+assert.strictEqual(typeof test_general.testAdjustExternalMemory(), 'number');


can we also test that the number is >0. I believe that we probably can't tell what the number will be, but I think we can depend on it being >0 ?

mhdawson · 2017-08-22T21:57:36Z

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

Added a simple wrapper around v8::Isolate::AdjustAmountOfExternalAllocatedMemory Fixes: nodejs#13928

Improved documentation based on pull request feedback Fixes: nodejs#13928

Changed napi_adjust_external_memory() signature to be more consistent with the rest of the API Fixes: nodejs#13928

Improved unit tests Fixes: nodejs#13928

BridgeAR · 2017-08-28T00:04:20Z

I guess this is ready to land?

benjamingr · 2017-08-30T19:31:06Z

Landed in c77e6d3

Added a wrapper around v8::Isolate::AdjustAmountOfExternalAllocatedMemory PR-URL: #14310 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Fixes: #13928

BridgeAR · 2017-08-30T19:32:17Z

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

BridgeAR · 2017-08-30T19:33:16Z

@benjamingr that was timing 😆

Added a wrapper around v8::Isolate::AdjustAmountOfExternalAllocatedMemory PR-URL: nodejs#14310 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Fixes: nodejs#13928

Added a wrapper around v8::Isolate::AdjustAmountOfExternalAllocatedMemory PR-URL: nodejs/node#14310 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Fixes: nodejs/node#13928

Added a wrapper around v8::Isolate::AdjustAmountOfExternalAllocatedMemory PR-URL: #14310 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Fixes: #13928

Added a wrapper around v8::Isolate::AdjustAmountOfExternalAllocatedMemory PR-URL: nodejs#14310 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Fixes: nodejs#13928

Added a wrapper around v8::Isolate::AdjustAmountOfExternalAllocatedMemory Backport-PR-URL: #19447 PR-URL: #14310 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Fixes: #13928

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Jul 16, 2017

chris--young mentioned this pull request Jul 16, 2017

N-API: expose function to suggest GC extends external memory #13928

Closed

addaleax approved these changes Jul 16, 2017

View reviewed changes

chris--young force-pushed the gc-hints branch from c12b52d to f64d5d8 Compare July 16, 2017 21:31

mhdawson reviewed Jul 18, 2017

View reviewed changes

TimothyGu approved these changes Jul 18, 2017

View reviewed changes

chris--young force-pushed the gc-hints branch from d394c4d to 963efa4 Compare July 18, 2017 01:21

cjihrig reviewed Jul 18, 2017

View reviewed changes

chris--young force-pushed the gc-hints branch from d61a58f to 268840f Compare July 23, 2017 22:40

jasnell approved these changes Jul 24, 2017

View reviewed changes

cjihrig requested changes Jul 24, 2017

View reviewed changes

digitalinfinity reviewed Aug 18, 2017

View reviewed changes

chris--young force-pushed the gc-hints branch from 268840f to b402fe8 Compare August 21, 2017 05:50

cjihrig approved these changes Aug 21, 2017

View reviewed changes

chris--young force-pushed the gc-hints branch from b402fe8 to 7476eb7 Compare August 22, 2017 18:13

mhdawson approved these changes Aug 22, 2017

View reviewed changes

Chris Young added 4 commits August 25, 2017 11:43

n-api: adds function to adjust external memory

a785d8c

Added a simple wrapper around v8::Isolate::AdjustAmountOfExternalAllocatedMemory Fixes: nodejs#13928

n-api: adds function to adjust external memory

77acfd5

Improved documentation based on pull request feedback Fixes: nodejs#13928

n-api: adds function to adjust external memory

fd40a07

Changed napi_adjust_external_memory() signature to be more consistent with the rest of the API Fixes: nodejs#13928

n-api: adds function to adjust external memory

81bc985

Improved unit tests Fixes: nodejs#13928

chris--young force-pushed the gc-hints branch from 12573e9 to 81bc985 Compare August 25, 2017 18:43

benjamingr approved these changes Aug 30, 2017

View reviewed changes

benjamingr closed this Aug 30, 2017

digitalinfinity mentioned this pull request Aug 31, 2017

AdjustExternalMemory nodejs/abi-stable-node#52

Closed

jackhorton mentioned this pull request Sep 7, 2017

napi_jsrt: Investigate usefulness of adjusting external memory nodejs/node-chakracore#388

Open

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the dont-land-on-v6.x label Sep 20, 2017

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: adds function to adjust external memory #14310

n-api: adds function to adjust external memory #14310

chris--young commented Jul 16, 2017

addaleax left a comment

addaleax Jul 16, 2017

addaleax Jul 16, 2017

mhdawson Jul 18, 2017

TimothyGu Jul 18, 2017

mhdawson Jul 18, 2017

mhdawson Jul 18, 2017

chris--young Jul 18, 2017

TimothyGu Jul 18, 2017

bnoordhuis Jul 18, 2017 •

edited

Loading

mhdawson commented Jul 18, 2017 •

edited

Loading

TimothyGu left a comment

TimothyGu Jul 18, 2017

cjihrig Jul 18, 2017

cjihrig Jul 24, 2017

chris--young Aug 21, 2017

cjihrig Jul 24, 2017

chris--young Jul 24, 2017

mhdawson commented Aug 17, 2017

chris--young commented Aug 17, 2017

digitalinfinity left a comment

mhdawson commented Aug 18, 2017

chris--young commented Aug 21, 2017

chris--young commented Aug 21, 2017

mhdawson left a comment

mhdawson Jul 18, 2017

mhdawson commented Aug 22, 2017

BridgeAR commented Aug 28, 2017

benjamingr commented Aug 30, 2017

BridgeAR commented Aug 30, 2017

BridgeAR commented Aug 30, 2017

n-api: adds function to adjust external memory #14310

n-api: adds function to adjust external memory #14310

Conversation

chris--young commented Jul 16, 2017

Checklist

Affected core subsystem(s)

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

mhdawson commented Jul 18, 2017 • edited Loading

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Aug 17, 2017

chris--young commented Aug 17, 2017

digitalinfinity left a comment

Choose a reason for hiding this comment

mhdawson commented Aug 18, 2017

chris--young commented Aug 21, 2017

chris--young commented Aug 21, 2017

mhdawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Aug 22, 2017

BridgeAR commented Aug 28, 2017

benjamingr commented Aug 30, 2017

BridgeAR commented Aug 30, 2017

BridgeAR commented Aug 30, 2017

bnoordhuis Jul 18, 2017 •

edited

Loading

mhdawson commented Jul 18, 2017 •

edited

Loading