Skip to content

Commit d8dda21

Browse files
RReverserRReverser
authored
[embind] Use a single invoker mechanism (#24577)
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 5669bed commit d8dda21

File tree

10 files changed

+188
-217
lines changed

10 files changed

+188
-217
lines changed

‎src/lib/libemval.js

Lines changed: 49 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -122,13 +122,6 @@ var LibraryEmVal = {
122122
_emval_new_u16string__deps: ['$Emval'],
123123
_emval_new_u16string: (v) => Emval.toHandle(UTF16ToString(v)),
124124

125-
_emval_take_value__deps: ['$Emval', '$requireRegisteredType'],
126-
_emval_take_value: (type, arg) => {
127-
type = requireRegisteredType(type, '_emval_take_value');
128-
var v = type['readValueFromPointer'](arg);
129-
return Emval.toHandle(v);
130-
},
131-
132125
#if SUPPORTS_GLOBALTHIS
133126
$emval_get_global: () => globalThis,
134127
#elif !DYNAMIC_EXECUTION
@@ -209,27 +202,6 @@ var LibraryEmVal = {
209202
return result;
210203
},
211204

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-
233205
_emval_equals__deps: ['$Emval'],
234206
_emval_equals: (first, second) => {
235207
first = Emval.toValue(first);
@@ -264,13 +236,6 @@ var LibraryEmVal = {
264236
return !object;
265237
},
266238

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-
274239
$emval_lookupTypes__deps: ['$requireRegisteredType'],
275240
$emval_lookupTypes: (argCount, argTypes) => {
276241
var a = new Array(argCount);
@@ -292,11 +257,12 @@ var LibraryEmVal = {
292257
return id;
293258
},
294259
295-
_emval_get_method_caller__deps: [
260+
_emval_create_invoker__deps: [
296261
'$emval_addMethodCaller', '$emval_lookupTypes',
297262
'$createNamedFunction', '$emval_returnValue',
263+
'$Emval', '$getStringOrSymbol',
298264
],
299-
_emval_get_method_caller: (argCount, argTypes, kind) => {
265+
_emval_create_invoker: (argCount, argTypes, kind) => {
300266
var GenericWireTypeSize = {{{ 2 * POINTER_SIZE }}};
301267
302268
var types = emval_lookupTypes(argCount, argTypes);
@@ -305,26 +271,38 @@ var LibraryEmVal = {
305271
306272
#if !DYNAMIC_EXECUTION
307273
var argN = new Array(argCount);
308-
var invokerFunction = (obj, func, destructorsRef, args) => {
274+
var invokerFunction = (handle, methodName, destructorsRef, args) => {
309275
var offset = 0;
310276
for (var i = 0; i < argCount; ++i) {
311277
argN[i] = types[i]['readValueFromPointer'](args + offset);
312278
offset += GenericWireTypeSize;
313279
}
314-
var rv = kind === /* CONSTRUCTOR */ 1 ? Reflect.construct(func, argN) : func.apply(obj, argN);
280+
var rv;
281+
switch (kind) {
282+
case {{{ cDefs['internal::EM_INVOKER_KIND::FUNCTION'] }}}:
283+
rv = Emval.toValue(handle).apply(null, argN);
284+
break;
285+
case {{{ cDefs['internal::EM_INVOKER_KIND::CONSTRUCTOR'] }}}:
286+
rv = Reflect.construct(Emval.toValue(handle), argN);
287+
break;
288+
case {{{ cDefs['internal::EM_INVOKER_KIND::CAST'] }}}:
289+
// no-op, just return the argument
290+
rv = argN[0];
291+
break;
292+
case {{{ cDefs['internal::EM_INVOKER_KIND::METHOD'] }}}:
293+
rv = Emval.toValue(handle)[getStringOrSymbol(methodName)](...argN);
294+
break;
295+
}
315296
return emval_returnValue(retType, destructorsRef, rv);
316297
};
317298
#else
318299
var functionBody =
319-
`return function (obj, func, destructorsRef, args) {\n`;
300+
`return function (handle, methodName, destructorsRef, args) {\n`;
320301
321302
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];
303+
var argsList = []; // 'arg0, arg1, arg2, ... , argN'
304+
var params = ['toValue', 'retType'];
305+
var args = [Emval.toValue, retType];
328306
for (var i = 0; i < argCount; ++i) {
329307
argsList.push(`arg${i}`);
330308
params.push(`argType${i}`);
@@ -333,7 +311,23 @@ var LibraryEmVal = {
333311
` var arg${i} = argType${i}.readValueFromPointer(args${offset ? '+' + offset : ''});\n`;
334312
offset += GenericWireTypeSize;
335313
}
336-
var invoker = kind === {{{ cDefs['internal::EM_METHOD_CALLER_KIND::CONSTRUCTOR'] }}} ? 'new func' : 'func.call';
314+
var invoker;
315+
switch (kind){
316+
case {{{ cDefs['internal::EM_INVOKER_KIND::FUNCTION'] }}}:
317+
invoker = 'toValue(handle)';
318+
break;
319+
case {{{ cDefs['internal::EM_INVOKER_KIND::CONSTRUCTOR'] }}}:
320+
invoker = 'new (toValue(handle))';
321+
break;
322+
case {{{ cDefs['internal::EM_INVOKER_KIND::CAST'] }}}:
323+
invoker = '';
324+
break;
325+
case {{{ cDefs['internal::EM_INVOKER_KIND::METHOD'] }}}:
326+
params.push('getStringOrSymbol');
327+
args.push(getStringOrSymbol);
328+
invoker = 'toValue(handle)[getStringOrSymbol(methodName)]';
329+
break;
330+
}
337331
functionBody +=
338332
` var rv = ${invoker}(${argsList.join(', ')});\n`;
339333
if (!retType.isVoid) {
@@ -351,14 +345,16 @@ var LibraryEmVal = {
351345
return emval_addMethodCaller(createNamedFunction(functionName, invokerFunction));
352346
},
353347
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);
348+
_emval_invoke__deps: ['$getStringOrSymbol', '$emval_methodCallers', '$Emval'],
349+
_emval_invoke: (caller, handle, methodName, destructorsRef, args) => {
350+
return emval_methodCallers[caller](handle, methodName, destructorsRef, args);
360351
},
361352
353+
// Same as `_emval_invoke`, just imported into Wasm under a different return type.
354+
// TODO: remove this if/when https://github.com/emscripten-core/emscripten/issues/20478 is fixed.
355+
_emval_invoke_i64__deps: ['_emval_invoke'],
356+
_emval_invoke_i64: '=__emval_invoke',
357+
362358
_emval_typeof__deps: ['$Emval'],
363359
_emval_typeof: (handle) => {
364360
handle = Emval.toValue(handle);

‎src/lib/libsigs.js

Lines changed: 3 additions & 7 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',
@@ -371,7 +368,6 @@ sigs = {
371368
_emval_run_destructors__sig: 'vp',
372369
_emval_set_property__sig: 'vppp',
373370
_emval_strictly_equals__sig: 'ipp',
374-
_emval_take_value__sig: 'ppp',
375371
_emval_throw__sig: 'ip',
376372
_emval_typeof__sig: 'pp',
377373
_gmtime_js__sig: 'vjp',

‎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)