Skip to content

Commit fb3ea98

Browse files
RReverserRReverser
committed
[embind] Use a single invoker mechanism
This is the next step in refactorings I started back in #20383 to merge all Embind generic method callers into a single mechanism. I never got around to landing this PR because I kept trying to split it into even smaller changes / steps, never found a good way to do so due to how entangled they were, got frustrated and left it alone. However, aside from being a nice-to-have optimisation or cleanup refactoring, it's also an easy way to deal with issues like #24547 which makes it a bit more pressing. Without this PR, we'd have to duplicate the `emval_as`+`emval_as_int64`+`emval_as_uint64` fix from #13889 for `emval_call` and `emval_call_method` as well, but after this PR everything goes through the same centralised implementation where we can deal with all Embind types in a consistent manner. There are some more things that I'd like to clean up here (including some of the complexity added by this very own PR), but in order to try and keep review scope smaller, I'm going to submit them separately. Fixes #24547.
1 parent bf5d63c commit fb3ea98

File tree

10 files changed

+192
-195
lines changed

10 files changed

+192
-195
lines changed

‎src/lib/libemval.js‎

Lines changed: 49 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -209,27 +209,6 @@ var LibraryEmVal = {
209209
return result;
210210
},
211211

212-
_emval_as__deps: ['$Emval', '$requireRegisteredType', '$emval_returnValue'],
213-
_emval_as: (handle, returnType, destructorsRef) => {
214-
handle = Emval.toValue(handle);
215-
returnType = requireRegisteredType(returnType, 'emval::as');
216-
return emval_returnValue(returnType, destructorsRef, handle);
217-
},
218-
219-
_emval_as_int64__deps: ['$Emval', '$requireRegisteredType'],
220-
_emval_as_int64: (handle, returnType) => {
221-
handle = Emval.toValue(handle);
222-
returnType = requireRegisteredType(returnType, 'emval::as');
223-
return returnType['toWireType'](null, handle);
224-
},
225-
226-
_emval_as_uint64__deps: ['$Emval', '$requireRegisteredType'],
227-
_emval_as_uint64: (handle, returnType) => {
228-
handle = Emval.toValue(handle);
229-
returnType = requireRegisteredType(returnType, 'emval::as');
230-
return returnType['toWireType'](null, handle);
231-
},
232-
233212
_emval_equals__deps: ['$Emval'],
234213
_emval_equals: (first, second) => {
235214
first = Emval.toValue(first);
@@ -264,13 +243,6 @@ var LibraryEmVal = {
264243
return !object;
265244
},
266245

267-
_emval_call__deps: ['$emval_methodCallers', '$Emval'],
268-
_emval_call: (caller, handle, destructorsRef, args) => {
269-
caller = emval_methodCallers[caller];
270-
handle = Emval.toValue(handle);
271-
return caller(null, handle, destructorsRef, args);
272-
},
273-
274246
$emval_lookupTypes__deps: ['$requireRegisteredType'],
275247
$emval_lookupTypes: (argCount, argTypes) => {
276248
var a = new Array(argCount);
@@ -292,11 +264,12 @@ var LibraryEmVal = {
292264
return id;
293265
},
294266
295-
_emval_get_method_caller__deps: [
267+
_emval_create_invoker__deps: [
296268
'$emval_addMethodCaller', '$emval_lookupTypes',
297269
'$createNamedFunction', '$emval_returnValue',
270+
'$Emval', '$getStringOrSymbol',
298271
],
299-
_emval_get_method_caller: (argCount, argTypes, kind) => {
272+
_emval_create_invoker: (argCount, argTypes, kind) => {
300273
var GenericWireTypeSize = {{{ 2 * POINTER_SIZE }}};
301274
302275
var types = emval_lookupTypes(argCount, argTypes);
@@ -305,26 +278,38 @@ var LibraryEmVal = {
305278
306279
#if !DYNAMIC_EXECUTION
307280
var argN = new Array(argCount);
308-
var invokerFunction = (obj, func, destructorsRef, args) => {
281+
var invokerFunction = (handle, methodName, destructorsRef, args) => {
309282
var offset = 0;
310283
for (var i = 0; i < argCount; ++i) {
311284
argN[i] = types[i]['readValueFromPointer'](args + offset);
312285
offset += GenericWireTypeSize;
313286
}
314-
var rv = kind === /* CONSTRUCTOR */ 1 ? Reflect.construct(func, argN) : func.apply(obj, argN);
287+
var rv;
288+
switch (kind) {
289+
case {{{ cDefs['internal::EM_INVOKER_KIND::FUNCTION'] }}}:
290+
rv = Emval.toValue(handle).apply(null, argN);
291+
break;
292+
case {{{ cDefs['internal::EM_INVOKER_KIND::CONSTRUCTOR'] }}}:
293+
rv = Reflect.construct(Emval.toValue(handle), argN);
294+
break;
295+
case {{{ cDefs['internal::EM_INVOKER_KIND::CAST'] }}}:
296+
// no-op, just return the argument
297+
rv = argN[0];
298+
break;
299+
case {{{ cDefs['internal::EM_INVOKER_KIND::METHOD'] }}}:
300+
rv = Emval.toValue(handle)[getStringOrSymbol(methodName)](...argN);
301+
break;
302+
}
315303
return emval_returnValue(retType, destructorsRef, rv);
316304
};
317305
#else
318306
var functionBody =
319-
`return function (obj, func, destructorsRef, args) {\n`;
307+
`return function (handle, methodName, destructorsRef, args) {\n`;
320308
321309
var offset = 0;
322-
var argsList = []; // 'obj?, arg0, arg1, arg2, ... , argN'
323-
if (kind === {{{ cDefs['internal::EM_METHOD_CALLER_KIND::FUNCTION'] }}}) {
324-
argsList.push('obj');
325-
}
326-
var params = ['retType'];
327-
var args = [retType];
310+
var argsList = []; // 'arg0, arg1, arg2, ... , argN'
311+
var params = ['toValue', 'retType'];
312+
var args = [Emval.toValue, retType];
328313
for (var i = 0; i < argCount; ++i) {
329314
argsList.push(`arg${i}`);
330315
params.push(`argType${i}`);
@@ -333,7 +318,23 @@ var LibraryEmVal = {
333318
` var arg${i} = argType${i}.readValueFromPointer(args${offset ? '+' + offset : ''});\n`;
334319
offset += GenericWireTypeSize;
335320
}
336-
var invoker = kind === {{{ cDefs['internal::EM_METHOD_CALLER_KIND::CONSTRUCTOR'] }}} ? 'new func' : 'func.call';
321+
var invoker;
322+
switch (kind){
323+
case {{{ cDefs['internal::EM_INVOKER_KIND::FUNCTION'] }}}:
324+
invoker = 'toValue(handle)';
325+
break;
326+
case {{{ cDefs['internal::EM_INVOKER_KIND::CONSTRUCTOR'] }}}:
327+
invoker = 'new (toValue(handle))';
328+
break;
329+
case {{{ cDefs['internal::EM_INVOKER_KIND::CAST'] }}}:
330+
invoker = '';
331+
break;
332+
case {{{ cDefs['internal::EM_INVOKER_KIND::METHOD'] }}}:
333+
params.push('getStringOrSymbol');
334+
args.push(getStringOrSymbol);
335+
invoker = 'toValue(handle)[getStringOrSymbol(methodName)]';
336+
break;
337+
}
337338
functionBody +=
338339
` var rv = ${invoker}(${argsList.join(', ')});\n`;
339340
if (!retType.isVoid) {
@@ -351,14 +352,16 @@ var LibraryEmVal = {
351352
return emval_addMethodCaller(createNamedFunction(functionName, invokerFunction));
352353
},
353354
354-
_emval_call_method__deps: ['$getStringOrSymbol', '$emval_methodCallers', '$Emval'],
355-
_emval_call_method: (caller, objHandle, methodName, destructorsRef, args) => {
356-
caller = emval_methodCallers[caller];
357-
objHandle = Emval.toValue(objHandle);
358-
methodName = getStringOrSymbol(methodName);
359-
return caller(objHandle, objHandle[methodName], destructorsRef, args);
355+
_emval_invoke__deps: ['$getStringOrSymbol', '$emval_methodCallers', '$Emval'],
356+
_emval_invoke: (caller, handle, methodName, destructorsRef, args) => {
357+
return emval_methodCallers[caller](handle, methodName, destructorsRef, args);
360358
},
361359
360+
// Same as `_emval_invoke`, just imported into Wasm under a different return type.
361+
// TODO: remove this if/when https://github.com/emscripten-core/emscripten/issues/20478 is fixed.
362+
_emval_invoke_i64__deps: ['_emval_invoke'],
363+
_emval_invoke_i64: '=__emval_invoke',
364+
362365
_emval_typeof__deps: ['$Emval'],
363366
_emval_typeof: (handle) => {
364367
handle = Emval.toValue(handle);

‎src/lib/libsigs.js‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -335,26 +335,23 @@ sigs = {
335335
_emscripten_thread_mailbox_await__sig: 'vp',
336336
_emscripten_thread_set_strongref__sig: 'vp',
337337
_emscripten_throw_longjmp__sig: 'v',
338-
_emval_as__sig: 'dppp',
339-
_emval_as_int64__sig: 'jpp',
340-
_emval_as_uint64__sig: 'jpp',
341338
_emval_await__sig: 'pp',
342-
_emval_call__sig: 'dpppp',
343-
_emval_call_method__sig: 'dppppp',
344339
_emval_coro_make_promise__sig: 'ppp',
345340
_emval_coro_suspend__sig: 'vpp',
341+
_emval_create_invoker__sig: 'pipi',
346342
_emval_decref__sig: 'vp',
347343
_emval_delete__sig: 'ipp',
348344
_emval_equals__sig: 'ipp',
349345
_emval_from_current_cxa_exception__sig: 'p',
350346
_emval_get_global__sig: 'pp',
351-
_emval_get_method_caller__sig: 'pipi',
352347
_emval_get_module_property__sig: 'pp',
353348
_emval_get_property__sig: 'ppp',
354349
_emval_greater_than__sig: 'ipp',
355350
_emval_in__sig: 'ipp',
356351
_emval_incref__sig: 'vp',
357352
_emval_instanceof__sig: 'ipp',
353+
_emval_invoke__sig: 'dppppp',
354+
_emval_invoke_i64__sig: 'jppppp',
358355
_emval_is_number__sig: 'ip',
359356
_emval_is_string__sig: 'ip',
360357
_emval_iter_begin__sig: 'pp',

‎src/struct_info_cxx.json‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,10 @@
2828
{
2929
"file": "emscripten/val.h",
3030
"defines": [
31-
"emscripten::internal::EM_METHOD_CALLER_KIND::FUNCTION",
32-
"emscripten::internal::EM_METHOD_CALLER_KIND::CONSTRUCTOR"
31+
"emscripten::internal::EM_INVOKER_KIND::FUNCTION",
32+
"emscripten::internal::EM_INVOKER_KIND::METHOD",
33+
"emscripten::internal::EM_INVOKER_KIND::CONSTRUCTOR",
34+
"emscripten::internal::EM_INVOKER_KIND::CAST"
3335
]
3436
}
3537
]

‎src/struct_info_generated.json‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -516,8 +516,10 @@
516516
"__WASI_RIGHTS_PATH_UNLINK_FILE": 67108864,
517517
"__WASI_RIGHTS_POLL_FD_READWRITE": 134217728,
518518
"__WASI_RIGHTS_SOCK_SHUTDOWN": 268435456,
519-
"internal::EM_METHOD_CALLER_KIND::CONSTRUCTOR": 1,
520-
"internal::EM_METHOD_CALLER_KIND::FUNCTION": 0
519+
"internal::EM_INVOKER_KIND::CAST": 3,
520+
"internal::EM_INVOKER_KIND::CONSTRUCTOR": 2,
521+
"internal::EM_INVOKER_KIND::FUNCTION": 0,
522+
"internal::EM_INVOKER_KIND::METHOD": 1
521523
},
522524
"structs": {
523525
"AudioParamFrame": {

‎src/struct_info_generated_wasm64.json‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -516,8 +516,10 @@
516516
"__WASI_RIGHTS_PATH_UNLINK_FILE": 67108864,
517517
"__WASI_RIGHTS_POLL_FD_READWRITE": 134217728,
518518
"__WASI_RIGHTS_SOCK_SHUTDOWN": 268435456,
519-
"internal::EM_METHOD_CALLER_KIND::CONSTRUCTOR": 1,
520-
"internal::EM_METHOD_CALLER_KIND::FUNCTION": 0
519+
"internal::EM_INVOKER_KIND::CAST": 3,
520+
"internal::EM_INVOKER_KIND::CONSTRUCTOR": 2,
521+
"internal::EM_INVOKER_KIND::FUNCTION": 0,
522+
"internal::EM_INVOKER_KIND::METHOD": 1
521523
},
522524
"structs": {
523525
"AudioParamFrame": {

0 commit comments

Comments
 (0)