[emval] Reuse method calling optimisation in regular calls #20383

RReverser · 2023-10-03T21:30:20Z

Method, function and constructor calls are very similar in nature but used different code paths: method calls have an optimisation where an optimised caller trampoline is generated once and reused for each function signature, whereas the other two serialized arguments and types every time.

There is no reason for them to diverge, so I made function calls and constructors reuse the same optimisation.

Results on a silly microbenchmark running a side-effect-ful JS function with 100 integer args in a loop:

Before

with dynamic execution:

as method: 224.504ns
as function: 5367.9ns

without dynamic execution:

as method: 845.044ns
as function: 5350.96ns

After

with dynamic execution:

as method: 262.74ns
as function: 246.593ns

without dynamic execution:

as method: 859.534ns
as function: 823.498ns

In real world the difference is unlikely to be perceptible, but at the very least this makes the code more consistent and deletes a bunch of duplication.

brendandahl

Looks good, minor nits.

brendandahl · 2023-10-17T21:35:25Z

system/include/emscripten/val.h

@@ -509,16 +480,28 @@ class val {
    return internal::_emval_delete(as_handle(), val_ref(property).as_handle());
  }

+  template<typename... Args>


Why was this moved?

Just to keep all variants / usages of internalCall together, I found it makes it easier to iterate on it when you can see all places that need to be updated together on one screen.

system/include/emscripten/val.h

src/embind/emval.js

RReverser · 2023-10-17T23:20:27Z

Addressed the feedback.

Reindent and change more concats to templates.

Before PR emscripten-core#20383 val's `call` and `operator()` had different behaviors where one would automatically destroy arguments after the call and the other didn't. After that PR, they both called destroy. The automatic destruction wasn't really documented anywhere and and led to some unexpected behavior. This changes it so neither automatically destroys the arguments which I think is more inline with the rest of embind where it's up to the user to handle object lifetimes. Fixes: emscripten-core#21016 and emscripten-core#20095

…21022) Before PR #20383 val's `call` and `operator()` had different behaviors where one would automatically destroy arguments after the call and the other didn't. After that PR, they both called destroy. The automatic destruction wasn't really documented anywhere and and led to some unexpected behavior. This changes it so neither automatically destroys the arguments which I think is more inline with the rest of embind where it's up to the user to handle object lifetimes. Fixes: #21016 and #20095

This is the next step in refactorings I started back in emscripten-core#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 emscripten-core#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 emscripten-core#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 emscripten-core#24547.

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.

RReverser requested review from sbc100 and brendandahl October 3, 2023 21:30

RReverser mentioned this pull request Oct 13, 2023

[emval] Move reference counting to C++ #20447

Draft

brendandahl requested changes Oct 17, 2023

View reviewed changes

RReverser added 11 commits October 18, 2023 00:29

Reuse caller optimisation in functions

49e6dc9

Move new_ to method callers too

5b3f0ec

Extract emval_returnValue helper

7a84d3d

Inline callAndDestruct

c1d36eb

Rebaseline sigs

e7ec23e

Remove emval_new from headers

30e3bf8

Exclude Reflect.construct fallback on modern browsers

d010347

Fix for C++11 compat

aeb8d93

Fix stylistic issues in codegen

bf013c9

Reindent and change more concats to templates.

Transform argsList to array

d5b9eff

Change AsCtor bool to enum

b4fef41

RReverser force-pushed the emval-optimised-call-new branch from 7ba7779 to b4fef41 Compare October 17, 2023 23:29

brendandahl approved these changes Oct 18, 2023

View reviewed changes

RReverser merged commit 12777ca into emscripten-core:main Oct 18, 2023

RReverser deleted the emval-optimised-call-new branch October 18, 2023 17:53

brendandahl mentioned this pull request Jan 4, 2024

embind: Val call invokers delete object arguments #21016

Closed

brendandahl mentioned this pull request Jan 5, 2024

embind: Don't automatically destroy arguments passed into val calls. #21022

Merged

stbergmann mentioned this pull request Sep 16, 2024

Embind regression: Leaked C++ object when calling from C++ to JS #22575

Open

RReverser mentioned this pull request Jun 16, 2025

[embind] Use a single invoker mechanism #24577

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[emval] Reuse method calling optimisation in regular calls #20383

[emval] Reuse method calling optimisation in regular calls #20383

Uh oh!

RReverser commented Oct 3, 2023

Uh oh!

brendandahl left a comment

Uh oh!

brendandahl Oct 17, 2023

Uh oh!

RReverser Oct 17, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RReverser commented Oct 17, 2023

Uh oh!

Uh oh!

[emval] Reuse method calling optimisation in regular calls #20383

[emval] Reuse method calling optimisation in regular calls #20383

Uh oh!

Conversation

RReverser commented Oct 3, 2023

Before

with dynamic execution:

without dynamic execution:

After

with dynamic execution:

without dynamic execution:

Uh oh!

brendandahl left a comment

Choose a reason for hiding this comment

Uh oh!

brendandahl Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

RReverser Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RReverser commented Oct 17, 2023

Uh oh!

Uh oh!

RReverser Oct 17, 2023 •

edited

Loading