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.

Sign up for GitHub

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

wasi: use WasmMemoryObject handle for perf #43544

Merged
merged 1 commit into from
Jun 25, 2022
+22 −129
Merged

wasi: use WasmMemoryObject handle for perf #43544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 0 additions & 16 deletions lib/wasi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,12 @@ const {
} = primordials;

const {
ERR_INVALID_ARG_TYPE,
ERR_WASI_ALREADY_STARTED
} = require('internal/errors').codes;
const {
emitExperimentalWarning,
kEmptyObject,
} = require('internal/util');
const { isArrayBuffer } = require('internal/util/types');
const {
validateArray,
validateBoolean,
Expand All @@ -39,20 +37,6 @@ function setupInstance(self, instance) {
validateObject(instance, 'instance');
validateObject(instance.exports, 'instance.exports');

// WASI::_SetMemory() in src/node_wasi.cc only expects that |memory| is
// an object. It will try to look up the .buffer property when needed
// and fail with UVWASI_EINVAL when the property is missing or is not
// an ArrayBuffer. Long story short, we don't need much validation here
// but we type-check anyway because it helps catch bugs in the user's
// code early.
validateObject(instance.exports.memory, 'instance.exports.memory');
if (!isArrayBuffer(instance.exports.memory.buffer)) {
throw new ERR_INVALID_ARG_TYPE(
'instance.exports.memory.buffer',
['WebAssembly.Memory'],
instance.exports.memory.buffer);
}

self[kInstance] = instance;
self[kSetMemory](instance.exports.memory);
}
Expand Down
30 changes: 12 additions & 18 deletions src/node_wasi.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,7 @@ inline void Debug(WASI* wasi, Args&&... args) {
} \
} while (0)


using v8::Array;
using v8::ArrayBuffer;
using v8::BackingStore;
using v8::BigInt;
using v8::Context;
Expand All @@ -89,7 +87,7 @@ using v8::Object;
using v8::String;
using v8::Uint32;
using v8::Value;

using v8::WasmMemoryObject;

static MaybeLocal<Value> WASIException(Local<Context> context,
int errorno,
Expand Down Expand Up @@ -1642,26 +1640,22 @@ void WASI::SockShutdown(const FunctionCallbackInfo<Value>& args) {

void WASI::_SetMemory(const FunctionCallbackInfo<Value>& args) {
WASI* wasi;
CHECK_EQ(args.Length(), 1);
CHECK(args[0]->IsObject());
ASSIGN_OR_RETURN_UNWRAP(&wasi, args.This());
wasi->memory_.Reset(wasi->env()->isolate(), args[0].As<Object>());
CHECK_EQ(args.Length(), 1);
if (!args[0]->IsWasmMemoryObject()) {
return node::THROW_ERR_INVALID_ARG_TYPE(
wasi->env(),
"\"instance.exports.memory\" property must be a WebAssembly.Memory "
"object");
}
wasi->memory_.Reset(wasi->env()->isolate(), args[0].As<WasmMemoryObject>());
}


uvwasi_errno_t WASI::backingStore(char** store, size_t* byte_length) {
Environment* env = this->env();
Local<Object> memory = PersistentToLocal::Strong(this->memory_);
Local<Value> prop;

if (!memory->Get(env->context(), env->buffer_string()).ToLocal(&prop))
return UVWASI_EINVAL;

if (!prop->IsArrayBuffer())
return UVWASI_EINVAL;

Local<ArrayBuffer> ab = prop.As<ArrayBuffer>();
std::shared_ptr<BackingStore> backing_store = ab->GetBackingStore();
Local<WasmMemoryObject> memory = PersistentToLocal::Strong(this->memory_);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny and very ignorable style nit:

Suggested change
Local<WasmMemoryObject> memory = PersistentToLocal::Strong(this->memory_);
Local<WasmMemoryObject> memory = PersistentToLocal::Strong(memory_);

More directly, why are we not going on step further and storing the std::shared_ptr<BackingStore> here? That should be even faster.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the arraybuffer gets detached every time the wasm grows its memory. very annoying problem, maybe it will be fixed one day: https://github.com/tc39/proposal-resizablearraybuffer#sync-up-capability-with-webassembly-memorygrow

std::shared_ptr<BackingStore> backing_store =
memory->Buffer()->GetBackingStore();
*byte_length = backing_store->ByteLength();
*store = static_cast<char*>(backing_store->Data());
CHECK_NOT_NULL(*store);
Expand Down
2 changes: 1 addition & 1 deletion src/node_wasi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ class WASI : public BaseObject,
inline void writeUInt64(char* memory, uint64_t value, uint32_t offset);
uvwasi_errno_t backingStore(char** store, size_t* byte_length);
uvwasi_t uvw_;
v8::Global<v8::Object> memory_;
v8::Global<v8::WasmMemoryObject> memory_;
uvwasi_mem_t alloc_info_;
size_t current_uvwasi_memory_ = 0;
};
Expand Down
53 changes: 6 additions & 47 deletions test/wasi/test-wasi-initialize-validation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ const bufferSource = fixtures.readSync('simple.wasm');

Object.defineProperty(instance, 'exports', {
get() {
return { _initialize: 5, memory: new Uint8Array() };
return {
_initialize: 5,
memory: new WebAssembly.Memory({ initial: 1 }),
};
},
});
assert.throws(
Expand All @@ -70,7 +73,7 @@ const bufferSource = fixtures.readSync('simple.wasm');
return {
_start() {},
_initialize() {},
memory: new Uint8Array(),
memory: new WebAssembly.Memory({ initial: 1 }),
};
}
});
Expand All @@ -97,55 +100,11 @@ const bufferSource = fixtures.readSync('simple.wasm');
() => { wasi.initialize(instance); },
{
code: 'ERR_INVALID_ARG_TYPE',
message: /"instance\.exports\.memory" property must be of type object/
message: /"instance\.exports\.memory" property must be a WebAssembly\.Memory object/
}
);
}

{
// Verify that a non-ArrayBuffer memory.buffer is rejected.
const wasi = new WASI({});
const wasm = await WebAssembly.compile(bufferSource);
const instance = await WebAssembly.instantiate(wasm);

Object.defineProperty(instance, 'exports', {
get() {
return {
_initialize() {},
memory: {},
};
}
});
// The error message is a little white lie because any object
// with a .buffer property of type ArrayBuffer is accepted,
// but 99% of the time a WebAssembly.Memory object is used.
assert.throws(
() => { wasi.initialize(instance); },
{
code: 'ERR_INVALID_ARG_TYPE',
message: /"instance\.exports\.memory\.buffer" property must be an WebAssembly\.Memory/
}
);
}

{
// Verify that an argument that duck-types as a WebAssembly.Instance
// is accepted.
const wasi = new WASI({});
const wasm = await WebAssembly.compile(bufferSource);
const instance = await WebAssembly.instantiate(wasm);

Object.defineProperty(instance, 'exports', {
get() {
return {
_initialize() {},
memory: { buffer: new ArrayBuffer(0) },
};
}
});
wasi.initialize(instance);
}

{
// Verify that a WebAssembly.Instance from another VM context is accepted.
const wasi = new WASI({});
Expand Down
50 changes: 3 additions & 47 deletions test/wasi/test-wasi-start-validation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ const bufferSource = fixtures.readSync('simple.wasm');

Object.defineProperty(instance, 'exports', {
get() {
return { memory: new Uint8Array() };
return { memory: new WebAssembly.Memory({ initial: 1 }) };
},
});
assert.throws(
Expand All @@ -70,7 +70,7 @@ const bufferSource = fixtures.readSync('simple.wasm');
return {
_start() {},
_initialize() {},
memory: new Uint8Array(),
memory: new WebAssembly.Memory({ initial: 1 }),
};
}
});
Expand All @@ -97,55 +97,11 @@ const bufferSource = fixtures.readSync('simple.wasm');
() => { wasi.start(instance); },
{
code: 'ERR_INVALID_ARG_TYPE',
message: /"instance\.exports\.memory" property must be of type object/
message: /"instance\.exports\.memory" property must be a WebAssembly\.Memory object/
}
);
}

{
// Verify that a non-ArrayBuffer memory.buffer is rejected.
const wasi = new WASI({});
const wasm = await WebAssembly.compile(bufferSource);
const instance = await WebAssembly.instantiate(wasm);

Object.defineProperty(instance, 'exports', {
get() {
return {
_start() {},
memory: {},
};
}
});
// The error message is a little white lie because any object
// with a .buffer property of type ArrayBuffer is accepted,
// but 99% of the time a WebAssembly.Memory object is used.
assert.throws(
() => { wasi.start(instance); },
{
code: 'ERR_INVALID_ARG_TYPE',
message: /"instance\.exports\.memory\.buffer" property must be an WebAssembly\.Memory/
}
);
}

{
// Verify that an argument that duck-types as a WebAssembly.Instance
// is accepted.
const wasi = new WASI({});
const wasm = await WebAssembly.compile(bufferSource);
const instance = await WebAssembly.instantiate(wasm);

Object.defineProperty(instance, 'exports', {
get() {
return {
_start() {},
memory: { buffer: new ArrayBuffer(0) },
};
}
});
wasi.start(instance);
}

{
// Verify that a WebAssembly.Instance from another VM context is accepted.
const wasi = new WASI({});
Expand Down