Skip to content

Update Node-API to latest from node.js - #70

Merged
bghgary merged 40 commits into
BabylonJS:mainfrom
carolhmj:update_napi
Feb 22, 2024
Merged

Update Node-API to latest from node.js#70
bghgary merged 40 commits into
BabylonJS:mainfrom
carolhmj:update_napi

Conversation

@carolhmj

@carolhmjcarolhmj commented Jan 5, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@CedricGuillemetCedricGuillemet mentioned this pull request Jan 30, 2024
Merged
@CedricGuillemet

CedricGuillemet commented Jan 30, 2024•
edited
Loading

Copy link
Copy Markdown
Collaborator

napi_create_external_arraybuffer checks the type of the array just created but it appears to not be a typed array. So napi_get_typedarray_info returns false and the array creation asserts.
image

If I replace this line : return napi_get_typedarray_info( env, *result, nullptr, nullptr, nullptr, result, nullptr); with return napi_ok; then everything works fine (validation tests and unittests pass). This replacement correspond to the code before this update.

@CedricGuillemet

CedricGuillemet commented Jan 30, 2024

Copy link
Copy Markdown
Collaborator

Regarding performance with BabylonJS/BabylonNative#1350 I get 2.7 seconds before this update 2.9-3 seconds with.

@CedricGuillemet

CedricGuillemet commented Jan 31, 2024

Copy link
Copy Markdown
Collaborator

It's actually not a good idea to update node-api without updating v8/jsc. So, it's better to mix #59 with this PR and changes in js_native_api_v8.cc are then not needed: v8 api is in sync with node-api.

But there are issues:

  • sandbox. When disabled, I get this error on win32
# Fatal error in , line 0
# Embedder-vs-V8 build configuration mismatch. On embedder side sandbox is DISABLED while on V8 side it's ENABLED.

But, there is this check in node-api:
https://github.com/nodejs/node/blob/4ab63db9e207b5dbb6c046946716a49b6e2c3e53/src/node_api.cc#L1053

  • this brings the 2nd issue regarding external buffer. It's currently disabled in this PR (see other comment) and it's a bit painful to put back in the code. I did try but I end up copying many files from node-api and it gets messy quite fast. Is there a way to just add what's needed here?

CedricGuillemet
CedricGuillemet reviewed Jan 31, 2024
View reviewed changes
Comment threadCore/Node-API/Source/js_native_api_v8.cc Outdated
@bghgarybghgary mentioned this pull request Feb 1, 2024
Merged
@CedricGuillemet

CedricGuillemet commented Feb 2, 2024

Copy link
Copy Markdown
Collaborator

@bghgary
I did another try at fixing the external buffer, with a copy this time. When testing ValidationTests, it crashes after 3 or 4 tests with this callstack :
image

Same crash happens with or without the self-deleting weak reference.

This was referenced Feb 2, 2024
Closed
Closed
@CedricGuillemet

CedricGuillemet commented Feb 6, 2024

Copy link
Copy Markdown
Collaborator

The crash I saw was because of async shaders. When running in release, everything is fine with Validation tests. Integration PR is here : BabylonJS/BabylonNative#1353

@CedricGuillemetCedricGuillemet mentioned this pull request Feb 8, 2024
Merged
bghgary
bghgary reviewed Feb 14, 2024
View reviewed changes
Comment threadCore/Node-API/Include/Shared/napi/js_native_api.h Outdated
bghgary
bghgary reviewed Feb 20, 2024
View reviewed changes
Comment threadCore/Node-API/CMakeLists.txt
Comment threadCore/Node-API/Include/Shared/napi/napi.h Outdated
@CedricGuillemetCedricGuillemet mentioned this pull request Feb 21, 2024
Closed
@bghgary
bghgary enabled auto-merge (squash) February 21, 2024 21:00
@bghgarybghgary changed the title Updating Node-APIUpdate Node-API to latest from node.jsFeb 21, 2024
@bghgary
bghgary disabled auto-merge February 21, 2024 21:00
@bghgary
bghgary merged commit 02055e8 into BabylonJS:mainFeb 22, 2024
This was referenced Feb 22, 2024
Closed
Closed
Closed
@bghgarybghgary mentioned this pull request Aug 21, 2026
Open
matthargett added a commit to rebeckerspecialties/JsRuntimeHost that referenced this pull request Sep 1, 2026
@matthargett
V8: napi_is_detached_arraybuffer reported every ArrayBuffer as detached
9adc234
The body was commented out and replaced with a bare `*result = true;` in BabylonJS#70
("Update Node-API to latest from node.js"), so the API answered "detached" for
every value handed to it, including a freshly created, perfectly live ArrayBuffer.
Anything branching on it -- an addon guarding a zero-copy path, or a
detach-then-verify sequence -- got the wrong answer with no error to notice.
The pinned V8 does declare ArrayBuffer::WasDetached() (v8-array-buffer.h), so the
commented-out implementation compiles as written; restore it.
Caught by NodeApi.DetachArrayBufferOrReportsUnsupported on Android_V8 and
Win32_x64_V8. Also record the napi_status of each napi_is_detached_arraybuffer call
in that test, so an engine that fails the query is not mistaken for one that
answered "detached".
Android arm64 V8: 22 tests / 20 passed / 2 intentional skips.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CedricGuillemetCedricGuillemetCedricGuillemet left review comments
@bghgarybghgarybghgary approved these changes
+1 more reviewer
@PolygonalSunPolygonalSunPolygonalSun left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@carolhmj@CedricGuillemet@bghgary@PolygonalSun