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

Expose v8::String::NewExternalOneByte and v8::String::NewExternalTwoByte to Node-API #48198

Closed
Brooooooklyn opened this issue May 27, 2023 · 9 comments
Assignees
Labels
feature request Issues that request new features to be added to Node.js. node-api Issues and PRs related to the Node-API.

Comments

@Brooooooklyn
Copy link

What is the problem this feature will solve?

Provide a faster way to create JavaScript string from static str

What is the feature you are proposing to solve the problem?

v8::String::NewExternalOneByte and v8::String::NewExternalTwoByte

What alternatives have you considered?

No response

@Brooooooklyn Brooooooklyn added the feature request Issues that request new features to be added to Node.js. label May 27, 2023
@VoltrexKeyva VoltrexKeyva added the node-api Issues and PRs related to the Node-API. label May 27, 2023
@bnoordhuis
Copy link
Member

Cross-engine portability is one of the n-api design goals and I don't think other engines have functionality that is similar enough. For instance, I'm fairly sure quickjs doesn't have a concept of non-owned strings.

I suppose n-api targeting other engines could copy the string to the JS heap but having APis with wildly different performance across engines isn't great.

@Flarna
Copy link
Member

Flarna commented May 30, 2023

Other engines could add such APIs if at some day node is seen as relevant user.
Not exposing any optimized APIs results in keeping NAPI slower than actually needed. It's quite a pain if one knows that moving from v8 to napi results in significant added overhead.
I'm not saying here that this added overhead is the standard case/relevant all the time.

@Jamesernator
Copy link

For something comparable, the current Chrome implementation of webassembly stringref proposal implements string.from_code_point for similar performance reasons.

It would probably make sense to at least have napi_create_string_code_point if WASM does adopt this as presumably all engines would have a faster path for the creation of such strings (which is expected as no decoding would be neccessary for such an instruction).

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Jun 1, 2023

We've had problems with Electron not supporting external Buffers: nodejs/node-addon-api#1257. I'm not opposed to this proposal though. I think it's fine to fall back to regular string creation.

@gabrielschulhof
Copy link
Contributor

BTW, is there an example of usage somewhere? Who manages the life cycle of the ExternalOneByteStringResource?

@Brooooooklyn
Copy link
Author

is there an example of usage somewhere?

I think the most common usage scenario is create JavaScript String from static string, like: denoland/deno#18051. There are also use cases in NAPI-RS:

@Brooooooklyn
Copy link
Author

We've had problems with Electron not supporting external Buffers

Yes, I handled it in NAPI-RS, too. I don't think it's a problem if we have a fallback strategy: https://github.com/napi-rs/napi-rs/blob/main/crates/napi/src/bindgen_runtime/js_values/buffer.rs#L245

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Jun 1, 2023

@Brooooooklyn

  let core_str =
    v8::String::new_external_onebyte_static(scope, b"core").unwrap();

It looks like the rust bindings for V8 have some mechanism for creating a static v8::ExternalStringResource to wrap the string literal. I don't think we can do that in Node-API without exposing v8::ExternalStringResource itself.

I think the only way to go is to heap-allocate v8::ExternalStringResource and tie it via a weak reference to the resulting string, to be freed when the string is garbage-collected. This would likely make it slower, not faster, and more memory intensive too, unless the string is sizable.

@gabrielschulhof gabrielschulhof self-assigned this Jun 2, 2023
@gabrielschulhof
Copy link
Contributor

We decided at the 2023-06-02 Node-API meeting that we will implement this as a best-effort optimization.

  • The function signatures will be exactly the same as napi_create_string_*
  • We will create a v8::ExternalStringResource for each string on the heap, and delete it using a weak reference to the resulting string.
  • We will fall back to napi_create_string_* if we cannot use external pointers (such as when pointercage is on).
  • We will document that optimized performance is not guaranteed, and because of that, modifying the string on the native side may or may not be reflected on the JS side.

@gabrielschulhof gabrielschulhof linked a pull request Jun 8, 2023 that will close this issue
RafaelGSS pushed a commit that referenced this issue Jul 3, 2023
Introduce APIs that allow for the creation of JavaScript strings without
copying the underlying native string into the engine. The APIs fall back
to regular string creation if the engine's external string APIs are
unavailable. In this case, an optional boolean out-parameter indicates
that the string was copied, and the optional finalizer is called if
given.

PR-URL: #48339
Fixes: #48198
Reviewed-By: Daeyeon Jeong <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
Introduce APIs that allow for the creation of JavaScript strings without
copying the underlying native string into the engine. The APIs fall back
to regular string creation if the engine's external string APIs are
unavailable. In this case, an optional boolean out-parameter indicates
that the string was copied, and the optional finalizer is called if
given.

PR-URL: nodejs#48339
Fixes: nodejs#48198
Reviewed-By: Daeyeon Jeong <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
Introduce APIs that allow for the creation of JavaScript strings without
copying the underlying native string into the engine. The APIs fall back
to regular string creation if the engine's external string APIs are
unavailable. In this case, an optional boolean out-parameter indicates
that the string was copied, and the optional finalizer is called if
given.

PR-URL: nodejs#48339
Fixes: nodejs#48198
Reviewed-By: Daeyeon Jeong <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
ruyadorno pushed a commit that referenced this issue Sep 8, 2023
Introduce APIs that allow for the creation of JavaScript strings without
copying the underlying native string into the engine. The APIs fall back
to regular string creation if the engine's external string APIs are
unavailable. In this case, an optional boolean out-parameter indicates
that the string was copied, and the optional finalizer is called if
given.

PR-URL: #48339
Fixes: #48198
Reviewed-By: Daeyeon Jeong <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
ruyadorno pushed a commit that referenced this issue Sep 13, 2023
Introduce APIs that allow for the creation of JavaScript strings without
copying the underlying native string into the engine. The APIs fall back
to regular string creation if the engine's external string APIs are
unavailable. In this case, an optional boolean out-parameter indicates
that the string was copied, and the optional finalizer is called if
given.

PR-URL: #48339
Fixes: #48198
Reviewed-By: Daeyeon Jeong <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
@RedYetiDev RedYetiDev moved this from Awaiting Triage to Triaged in Node.js feature requests Jun 26, 2024
@RedYetiDev RedYetiDev moved this from Triaged to Done in Node.js feature requests Aug 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants
@bnoordhuis @gabrielschulhof @Brooooooklyn @Jamesernator @Flarna @VoltrexKeyva and others