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.

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

Remove wasteful allocation in neon_runtime::napi::call::get #530

Closed
goto-bus-stop opened this issue Jun 19, 2020 · 3 comments
Closed

Remove wasteful allocation in neon_runtime::napi::call::get #530

goto-bus-stop opened this issue Jun 19, 2020 · 3 comments

Comments

@goto-bus-stop
Copy link
Member

As of #507, getting a specific argument N in a function call internally creates a temporary Vec that N-API can store the arguments up to and includig N. It's done this way because napi_get_cb_info doesn't let you get a particular argument, but only a list of the arguments.

The end result is that code like:

cx.argument::<JsString>(1); // allocates vec![napi_value; 1]
cx.argument::<JsString>(2); // allocates vec![napi_value; 2]
cx.argument::<JsString>(3); // allocates vec![napi_value; 3]

There could be several ways to improve this:

  • Use a fixed array for low numbers of arguments, like [napi_value; 10], and only use a dynamic Vec<> when a higher N is requested. This avoids the allocations in the common case which are (probably) the most expensive.
  • Get all of the arguments in a Vec<> once, and store it in the FunctionCallbackInfo on the Rust side. This might be a bit harder but it would eliminate the waste entirely in the N-API runtime.
  • Propose an N-API addition in Node.js core that can retrieve a single particular argument 😇
  • Maybe some other way?
@kjvalencik
Copy link
Member

#2 to seems like a possible option. Drop the #[repr(C)] from CallbackInfo and for a proper constructor on it that proactively calls napi_get_cb_info for the arguments (or maybe lazily calls it, initiated as an Option).

I'm not sure how many touch points are implicitly converting to that type or transmuting to it.

@kjvalencik
Copy link
Member

Taking another look:

  • CallbackInfo is pub(crate). Breaking changes are okay.
  • neon_runtime::call::get is only called in two places. CallbackInfo::get and CallbackInfo::require
  • CallbackInfo::get is only called in one place. CallContext::argument_opt
  • CallbackInfo::require is only called in one place. CallContext::argument

Given the limited scope of these changes and that all calls are abstracted by CallContext, I think the easiest change is to lazily cache the arguments in an Option on CallContext.

@kjvalencik
Copy link
Member

#534

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants