[OV JS] Improve Core.importModel implementation: SharedStreamBuffer + variant source #33658

Nishant-ZFYII · 2026-01-17T05:35:56Z

What / Why

This refactors the Node.js bindings for Core.importModelSync() and Core.importModel().

The async Buffer path was still going through std::string → std::stringstream, which adds extra copies. Also, the async worker didn’t cleanly separate “import from stream bytes” vs “import from tensor”, so it was easy to end up treating tensor data like a stream. This change switches Buffer imports to ov::SharedStreamBuffer and makes the async worker explicitly dispatch based on input type.

No JS API changes — just internal implementation cleanup.

What changed

Buffer imports (sync + async) now use ov::SharedStreamBuffer + std::istream instead of building a std::stringstream.
ImportModelContext now stores the input as std::variant<std::monostate, BufferSource, TensorSource>.
- BufferSource keeps a persistent reference to the JS Buffer so it can’t be GC’d while the worker is running, and wraps the underlying bytes with ov::SharedStreamBuffer.
- TensorSource keeps a persistent reference to the JS object backing TensorWrap, and stores ov::Tensor directly.
importModelThread uses std::visit to call the right ov::Core::import_model overload (stream vs tensor).
Exceptions in the native worker are surfaced to JS by rejecting the Promise with an Error.
Updated the unit test that checks signature-validation error strings (the allowed-signatures output changed with the refactor).

Files changed

src/bindings/js/node/include/core_wrap.hpp
src/bindings/js/node/src/core_wrap.cpp
src/bindings/js/node/tests/unit/basic.test.js

Checks

Ran:

npm run lint
npm run format
npm test

CC: @almilosz @Retribution98

…or sources - Add required includes: <variant>, <istream>, <memory>, shared_buffer.hpp - Replace std::stringstream with std::variant<monostate, BufferSource, TensorSource> - BufferSource: pins JS Buffer with ObjectReference, stores SharedStreamBuffer - TensorSource: pins JS TensorWrap, stores ov::Tensor directly - Add _error_msg field for proper async error propagation - Use ov::AnyMap instead of std::map<std::string, ov::Any> Part of issue openvinotoolkit#33601: Improve implementation of Node.js API Core.import_model()

- Add required includes: <type_traits>, tensor.hpp, shared_buffer.hpp - Replace direct _stream access with std::visit over source variant - BufferSource: create local std::istream over SharedStreamBuffer - TensorSource: call import_model(tensor, ...) directly - Add try/catch with proper Promise rejection on error - Handle both std::exception and unknown exceptions Part of issue openvinotoolkit#33601: Improve implementation of Node.js API Core.import_model()

…nsor support - Replace std::string + std::stringstream with ov::SharedStreamBuffer (zero-copy) - Add Tensor input support using cast_to_tensor and direct tensor overload - Use ov::js::validate pattern for consistent argument validation - Use ov::AnyMap for config consistency - Proper static_cast for Buffer length to size_t Part of issue openvinotoolkit#33601: Improve implementation of Node.js API Core.import_model()

- Use std::make_unique for RAII safety (no leak if setup throws) - Add Tensor input support: pin TensorWrap with ObjectReference - Buffer input: pin JS Buffer, use SharedStreamBuffer (zero-copy) - Store source in variant, worker thread dispatches via std::visit - Consistent ov::AnyMap usage for config Part of issue openvinotoolkit#33601: Improve implementation of Node.js API Core.import_model()

Auto-format core_wrap.hpp and core_wrap.cpp to match project code style.

- importModelSync now uses ov::js::validate() and reports standardized parameter mismatch errors - Update basic.test.js to assert on the new stable error message instead of legacy per-arg type strings Part of issue openvinotoolkit#33601: Improve implementation of Node.js API Core.import_model()

almilosz · 2026-01-21T08:53:21Z

build_jenkins

almilosz

Thanks for the PR! I left a few comments

almilosz · 2026-01-21T09:29:16Z

src/bindings/js/node/src/core_wrap.cpp

    auto callback = [](Napi::Env env, Napi::Function, ImportModelContext* context) {
-        context->deferred.Resolve(cpp_to_js(env, context->_compiled_model));
+        if (!context->_error_msg.empty()) {
+            context->deferred.Reject(Napi::Error::New(env, context->_error_msg).Value());


Do we have a test for such scenario?

Yes I added a test case for this.
There’s a new unit test that passes an invalid/empty buffer and asserts Core.importModel() returns a Promise that rejects.

it("importModel rejects promise on invalid buffer", async () => { const invalid = Buffer.alloc(0); await assert.rejects(core.importModel(invalid, "CPU"), Error); });

almilosz · 2026-01-21T10:53:35Z

src/bindings/js/node/tests/unit/basic.test.js

      assert.throws(
        () => core.importModelSync(model, "CPU"),
-        /'importModelSync' method called with incorrect parameters./,
+        /'importModelSync' method called with incorrect parameters\./,


Suggested change

/'importModelSync' method called with incorrect parameters\./,

/'importModelSync' method called with incorrect parameters./,

looks unnecessary

I’ve reverted/simplified those assertions in basic.test.js to drop the \. escape .

I originally escaped the dot to match the trailing period literally when tightening the tests around the validation error output, but it doesn’t really add signal and just makes the check more brittle/noisy. This should be cleaner now.

almilosz · 2026-01-21T10:59:45Z

src/bindings/js/node/src/core_wrap.cpp

+
+        context->_compiled_model = std::visit(
+            [&](auto& src) -> ov::CompiledModel {
+                using T = std::decay_t<decltype(src)>;


For better readability/consistency you can use std::visit(overloaded as here

Thanks . Updated to std::visit().

almilosz · 2026-01-21T11:11:10Z

src/bindings/js/node/src/core_wrap.cpp

-            // Returns a Promise to JS. Method import_model() is performed on additional thread.
-            return context_data->deferred.Promise();
+            context_data->nativeThread = std::thread(importModelThread, context_data.get(), std::ref(_mutex));
+            auto* raw = context_data.release();


If we need raw pointer anyway, let's go back to auto context_data = new ImportModelContext(env, _core);
Or is there a different reason for this change?

Good point — no other reason.

I used the make_unique() + release() approach mainly to avoid leaks if something fails during setup, but since we end up needing a raw pointer anyway, it’s not the clearest pattern here.

I’ve switched it back to new ImportModelContext(...) and kept the same safety. This should be easier to follow and consistent with the rest of the file.

almilosz · 2026-01-21T11:16:07Z

src/bindings/js/node/src/core_wrap.cpp

+            buf_src.data = reinterpret_cast<const char*>(buf.Data());
+            buf_src.size = static_cast<size_t>(buf.Length());


Those are used only to create SharedStreamBuffer and can be removed from ImportModelContext::BufferSource

almilosz · 2026-01-21T11:17:53Z

src/bindings/js/node/src/core_wrap.cpp

+            ov::SharedStreamBuffer shared_buffer(reinterpret_cast<const char*>(model_data.Data()),
+                                                 static_cast<size_t>(model_data.Length()));


Suggested change

ov::SharedStreamBuffer shared_buffer(reinterpret_cast<const char*>(model_data.Data()),

static_cast<size_t>(model_data.Length()));

ov::SharedStreamBuffer shared_buffer(model_data.Data(), model_data.Length());

Yes I made those changes.

- Remove explicit casts in SharedStreamBuffer construction - Use overloaded helper for std::visit instead of if constexpr chain - Remove redundant data/size fields from BufferSource struct - Use raw new with try-catch guard instead of make_unique + release - Remove unnecessary regex escape in test assertions - Add test for importModel promise rejection on invalid buffer

Nishant-ZFYII added 6 commits January 17, 2026 10:16

[OV JS] Apply clang-format-18 formatting

7877ee7

Auto-format core_wrap.hpp and core_wrap.cpp to match project code style.

Nishant-ZFYII requested a review from a team as a code owner January 17, 2026 05:35

github-actions bot added the category: JS API OpenVino JS API Bindings label Jan 17, 2026

sys-openvino-ci added the ExternalPR External contributor label Jan 17, 2026

Merge branch 'master' into js-importmodel-sharedstreambuffer-variant

5f4758e

Nishant-ZFYII mentioned this pull request Jan 18, 2026

[JS API] Add import_model(tensor) to Node.js API #33401

Merged

almilosz reviewed Jan 21, 2026

View reviewed changes

	/'importModelSync' method called with incorrect parameters\./,
	/'importModelSync' method called with incorrect parameters./,

		buf_src.data = reinterpret_cast<const char*>(buf.Data());
		buf_src.size = static_cast<size_t>(buf.Length());

		ov::SharedStreamBuffer shared_buffer(reinterpret_cast<const char*>(model_data.Data()),
		static_cast<size_t>(model_data.Length()));

	ov::SharedStreamBuffer shared_buffer(reinterpret_cast<const char*>(model_data.Data()),
	static_cast<size_t>(model_data.Length()));
	ov::SharedStreamBuffer shared_buffer(model_data.Data(), model_data.Length());

[OV JS] Improve Core.importModel implementation: SharedStreamBuffer + variant source #33658

Are you sure you want to change the base?

[OV JS] Improve Core.importModel implementation: SharedStreamBuffer + variant source #33658

Conversation

Nishant-ZFYII commented Jan 17, 2026

What / Why

What changed

Files changed

Checks

Uh oh!

almilosz commented Jan 21, 2026

Uh oh!

almilosz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants