From 08535fb8af25ac2c605c0c94b43999747e9376e7 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Thu, 12 Oct 2023 23:48:00 +0100 Subject: [PATCH 01/13] Move reference counting to C++ This allows to completely optimise away the extra allocation / wrapper when values are local, and generally avoids crossing the JS<>Wasm boundary as often. --- src/embind/emval.js | 17 +++++------------ system/include/emscripten/val.h | 32 ++++++++++++++++---------------- 2 files changed, 21 insertions(+), 28 deletions(-) diff --git a/src/embind/emval.js b/src/embind/emval.js index 52b3d7b7b1e19..695e9856653d9 100644 --- a/src/embind/emval.js +++ b/src/embind/emval.js @@ -67,7 +67,7 @@ var LibraryEmVal = { if (!handle) { throwBindingError('Cannot use deleted val. handle = ' + handle); } - return emval_handles.get(handle).value; + return emval_handles.get(handle); }, toHandle: (value) => { @@ -77,22 +77,15 @@ var LibraryEmVal = { case true: return 3; case false: return 4; default:{ - return emval_handles.allocate({refcount: 1, value: value}); + return emval_handles.allocate(value); } } } }, - _emval_incref__deps: ['$emval_handles'], - _emval_incref: (handle) => { - if (handle > 4) { - emval_handles.get(handle).refcount += 1; - } - }, - - _emval_decref__deps: ['$emval_handles'], - _emval_decref: (handle) => { - if (handle >= emval_handles.reserved && 0 === --emval_handles.get(handle).refcount) { + _emval_free__deps: ['$emval_handles'], + _emval_free: (handle) => { + if (handle >= emval_handles.reserved) { emval_handles.free(handle); } }, diff --git a/system/include/emscripten/val.h b/system/include/emscripten/val.h index bd35eeff03243..0fd5f24f21afc 100644 --- a/system/include/emscripten/val.h +++ b/system/include/emscripten/val.h @@ -48,8 +48,7 @@ typedef struct _EM_METHOD_CALLER* EM_METHOD_CALLER; typedef double EM_GENERIC_WIRE_TYPE; typedef const void* EM_VAR_ARGS; -void _emval_incref(EM_VAL value); -void _emval_decref(EM_VAL value); +void _emval_free(EM_VAL object); void _emval_run_destructors(EM_DESTRUCTORS handle); @@ -386,22 +385,13 @@ class val { : val(internal::_emval_new_cstring(v)) {} - // Note: unlike other constructors, this doesn't use as_handle() because - // it just moves a value and doesn't need to go via incref/decref. - // This means it's safe to move values across threads - an error will - // only arise if you access or free it from the wrong thread later. - val(val&& v) : handle(v.handle), thread(v.thread) { - v.handle = 0; - } - - val(const val& v) : val(v.as_handle()) { - internal::_emval_incref(handle); + val(const val& v) : handle(v.handle), thread(v.thread), refcount(v.refcount) { + ++*refcount; } ~val() { - if (handle) { - internal::_emval_decref(as_handle()); - handle = 0; + if (--*refcount == 0) { + internal::_emval_free(as_handle()); } } @@ -621,8 +611,18 @@ class val { return v; } + static size_t *make_refcount() { + // Note: C++ standard doesn't allow optimising out new/delete pairs, + // but malloc/free pairs get optimised out by LLVM completely if all + // your values are local (if you don't leak / store val elsewhere). + size_t *refcount = (size_t*)malloc(sizeof(size_t)); + *refcount = 1; + return refcount; + } + pthread_t thread; EM_VAL handle; + size_t *refcount = make_refcount(); friend struct internal::BindingType; }; @@ -663,7 +663,7 @@ struct BindingType::value && typedef EM_VAL WireType; static WireType toWireType(const val& v) { EM_VAL handle = v.as_handle(); - _emval_incref(handle); + ++*v.refcount; return handle; } static val fromWireType(WireType v) { From f9480ce72500c4a3b9810d5c499a10f2b8eae567 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Thu, 12 Oct 2023 23:48:00 +0100 Subject: [PATCH 02/13] Use non-copied binding for emval --- src/embind/embind.js | 8 ++------ system/include/emscripten/val.h | 4 +--- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/src/embind/embind.js b/src/embind/embind.js index 1b7e9f8334716..e88d332353f54 100644 --- a/src/embind/embind.js +++ b/src/embind/embind.js @@ -659,17 +659,13 @@ var LibraryEmbind = { }, _embind_register_emval__deps: [ - '_emval_decref', '$Emval', + '$Emval', '$readLatin1String', '$registerType', '$simpleReadValueFromPointer'], _embind_register_emval: (rawType, name) => { name = readLatin1String(name); registerType(rawType, { name, - 'fromWireType': (handle) => { - var rv = Emval.toValue(handle); - __emval_decref(handle); - return rv; - }, + 'fromWireType': (handle) => Emval.toValue(handle), 'toWireType': (destructors, value) => Emval.toHandle(value), 'argPackAdvance': GenericWireTypeSize, 'readValueFromPointer': simpleReadValueFromPointer, diff --git a/system/include/emscripten/val.h b/system/include/emscripten/val.h index 0fd5f24f21afc..ef8db5221b664 100644 --- a/system/include/emscripten/val.h +++ b/system/include/emscripten/val.h @@ -662,9 +662,7 @@ struct BindingType::value && !std::is_const::value>::type> { typedef EM_VAL WireType; static WireType toWireType(const val& v) { - EM_VAL handle = v.as_handle(); - ++*v.refcount; - return handle; + return v.as_handle(); } static val fromWireType(WireType v) { return val::take_ownership(v); From 032ea665d226a10d44f95594219cd8741ecbe34d Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Thu, 12 Oct 2023 23:48:00 +0100 Subject: [PATCH 03/13] Fix _emval_run_destructors --- src/embind/emval.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/embind/emval.js b/src/embind/emval.js index 695e9856653d9..d0a21a742a8ed 100644 --- a/src/embind/emval.js +++ b/src/embind/emval.js @@ -90,11 +90,11 @@ var LibraryEmVal = { } }, - _emval_run_destructors__deps: ['_emval_decref', '$Emval', '$runDestructors'], + _emval_run_destructors__deps: ['_emval_free', '$Emval', '$runDestructors'], _emval_run_destructors: (handle) => { var destructors = Emval.toValue(handle); runDestructors(destructors); - __emval_decref(handle); + __emval_free(handle); }, _emval_new_array__deps: ['$Emval'], From 0c33c8fec5207b363cc797fc6dcb23411ab919d8 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Thu, 12 Oct 2023 23:48:00 +0100 Subject: [PATCH 04/13] Extract val_metadata with all info This reduces size of class val to a single pointer, and still gets optimised out as an allocation. --- system/include/emscripten/val.h | 68 ++++++++++++++++++++++----------- 1 file changed, 45 insertions(+), 23 deletions(-) diff --git a/system/include/emscripten/val.h b/system/include/emscripten/val.h index ef8db5221b664..deab9ba2b4988 100644 --- a/system/include/emscripten/val.h +++ b/system/include/emscripten/val.h @@ -286,6 +286,46 @@ struct MethodCaller { } }; +struct val_metadata { +public: + val_metadata(EM_VAL handle) + : refcount(1) + , thread(pthread_self()) + , handle(handle) + {} + + // automatically deletes copy constructors and assignment operators as well + val_metadata(val_metadata&&) = delete; + + val_metadata* inc_ref() { + ++refcount; + return this; + } + + void dec_ref() { + if (--refcount == 0) { + delete this; + } + } + + constexpr EM_VAL as_handle() const { +#ifdef _REENTRANT + assert(pthread_equal(thread, pthread_self()) && "val accessed from wrong thread"); +#endif + return handle; + } + +private: + size_t refcount; + pthread_t thread; + EM_VAL handle; + + // should be only accessible from dec_ref + ~val_metadata() { + _emval_free(handle); + } +}; + } // end namespace internal #define EMSCRIPTEN_SYMBOL(name) \ @@ -385,21 +425,14 @@ class val { : val(internal::_emval_new_cstring(v)) {} - val(const val& v) : handle(v.handle), thread(v.thread), refcount(v.refcount) { - ++*refcount; - } + val(const val& v) : data(v.data->inc_ref()) {} ~val() { - if (--*refcount == 0) { - internal::_emval_free(as_handle()); - } + data->dec_ref(); } EM_VAL as_handle() const { -#ifdef _REENTRANT - assert(pthread_equal(thread, pthread_self()) && "val accessed from wrong thread"); -#endif - return handle; + return data->as_handle(); } val& operator=(val&& v) & { @@ -587,7 +620,7 @@ class val { private: // takes ownership, assumes handle already incref'd and lives on the same thread explicit val(EM_VAL handle) - : handle(handle), thread(pthread_self()) + : data(new internal::val_metadata(handle)) {} template @@ -611,18 +644,7 @@ class val { return v; } - static size_t *make_refcount() { - // Note: C++ standard doesn't allow optimising out new/delete pairs, - // but malloc/free pairs get optimised out by LLVM completely if all - // your values are local (if you don't leak / store val elsewhere). - size_t *refcount = (size_t*)malloc(sizeof(size_t)); - *refcount = 1; - return refcount; - } - - pthread_t thread; - EM_VAL handle; - size_t *refcount = make_refcount(); + internal::val_metadata* data; friend struct internal::BindingType; }; From 21ed8039140e3090cef72ae41e0d7f21ee3cd3fa Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Thu, 12 Oct 2023 23:48:00 +0100 Subject: [PATCH 05/13] We don't pass null handles anymore --- src/embind/emval.js | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/embind/emval.js b/src/embind/emval.js index d0a21a742a8ed..9ceef12b52009 100644 --- a/src/embind/emval.js +++ b/src/embind/emval.js @@ -63,12 +63,7 @@ var LibraryEmVal = { $Emval__deps: ['$emval_handles', '$throwBindingError', '$init_emval'], $Emval: { - toValue: (handle) => { - if (!handle) { - throwBindingError('Cannot use deleted val. handle = ' + handle); - } - return emval_handles.get(handle); - }, + toValue: (handle) => emval_handles.get(handle), toHandle: (value) => { switch (value) { From de26e591eeb03c146765caba4fc5a054bb5b2ae7 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Thu, 12 Oct 2023 23:48:00 +0100 Subject: [PATCH 06/13] Move reserved handle check to C++ too This allows to completely eliminate statements like `val v;` as compiler can statically prove it doesn't need _emval_free. --- src/embind/emval.js | 4 +--- system/include/emscripten/val.h | 11 +++++++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/embind/emval.js b/src/embind/emval.js index 9ceef12b52009..e8967b1e26b3b 100644 --- a/src/embind/emval.js +++ b/src/embind/emval.js @@ -80,9 +80,7 @@ var LibraryEmVal = { _emval_free__deps: ['$emval_handles'], _emval_free: (handle) => { - if (handle >= emval_handles.reserved) { - emval_handles.free(handle); - } + emval_handles.free(handle); }, _emval_run_destructors__deps: ['_emval_free', '$Emval', '$runDestructors'], diff --git a/system/include/emscripten/val.h b/system/include/emscripten/val.h index deab9ba2b4988..251743f30047f 100644 --- a/system/include/emscripten/val.h +++ b/system/include/emscripten/val.h @@ -38,9 +38,10 @@ void _emval_register_symbol(const char*); enum { _EMVAL_UNDEFINED = 1, - _EMVAL_NULL = 2, - _EMVAL_TRUE = 3, - _EMVAL_FALSE = 4 + _EMVAL_NULL, + _EMVAL_TRUE, + _EMVAL_FALSE, + _EMVAL_UNRESERVED_START }; typedef struct _EM_DESTRUCTORS* EM_DESTRUCTORS; @@ -322,7 +323,9 @@ struct val_metadata { // should be only accessible from dec_ref ~val_metadata() { - _emval_free(handle); + if (handle >= EM_VAL(internal::_EMVAL_UNRESERVED_START)) { + internal::_emval_free(handle); + } } }; From 5957965e8289cf344fd297da394efc8ce4035eee Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Thu, 12 Oct 2023 23:48:01 +0100 Subject: [PATCH 07/13] Use elimination-friendly new and delete C++ doesn't allow elimination of new/delete pairs, but in C malloc/free pairs can be and are eliminated by LLVM. In theory Clang's __builtin_operator_{new,delete} could also help here, but in my experiments it wasn't optimised out while malloc/free still were. I don't think we care about calling into user's overrides in this class too much, and the optimisation is valuable enough, so just using standard C funcs. --- system/include/emscripten/val.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/system/include/emscripten/val.h b/system/include/emscripten/val.h index 251743f30047f..6222f3a2c2f77 100644 --- a/system/include/emscripten/val.h +++ b/system/include/emscripten/val.h @@ -316,6 +316,16 @@ struct val_metadata { return handle; } + // Elimination-friendly overrides of operator new and delete. + + void* operator new(size_t count) noexcept { + return malloc(count * sizeof(val_metadata)); + } + + void operator delete(void* ptr) noexcept { + free(ptr); + } + private: size_t refcount; pthread_t thread; From e445e05092cb4bb81b1a424dea594aee7d5bea1b Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Thu, 12 Oct 2023 23:48:01 +0100 Subject: [PATCH 08/13] Handle out of memory condition --- system/include/emscripten/val.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/system/include/emscripten/val.h b/system/include/emscripten/val.h index 6222f3a2c2f77..fc826ee1b001f 100644 --- a/system/include/emscripten/val.h +++ b/system/include/emscripten/val.h @@ -317,9 +317,16 @@ struct val_metadata { } // Elimination-friendly overrides of operator new and delete. + // These intentionally do less work than the default implementations, + // which need to support custom handlers for out-of-memory conditions, + // while malloc/free pairs get inlined and can be and are eliminated + // by LLVM easily. + // Since most our values are temporary, we really want this optimisation. void* operator new(size_t count) noexcept { - return malloc(count * sizeof(val_metadata)); + auto ptr = malloc(count * sizeof(val_metadata)); + if (!ptr) abort(); + return ptr; } void operator delete(void* ptr) noexcept { From 81fdb3c9984439941248d143b3e8b6c973ecee72 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Thu, 12 Oct 2023 23:51:13 +0100 Subject: [PATCH 09/13] Add test --- test/test_other.py | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/test/test_other.py b/test/test_other.py index 6f88b545343b9..27b9bfe60efbc 100644 --- a/test/test_other.py +++ b/test/test_other.py @@ -14015,3 +14015,40 @@ def test_hello_world_argv(self): def test_arguments_global(self): self.emcc(test_file('hello_world_argv.c'), ['-sENVIRONMENT=web', '-sSTRICT', '--closure=1', '-O2']) + + def test_emval_allocation_optimisations(self): + # Abort on any attempts to use dynamic allocation in runtime. + # This way we can check that LLVM optimised them all away. + # It's possible (but very unlikely) that in some future LLVM + # will regress in this optimisation, but it's more likely that + # we modify `val` in some way that makes those optimisations + # impossible - this is what the test is designed to help catch. + create_file('no_alloc.c', r''' + #include + + void *malloc(size_t size) { + // Ensure all calls to malloc were compiled away when forbidden. + abort(); + } + + void free(void *ptr) { + // Same for free. + abort(); + } + ''') + # Add this allocation implementation to compilation. + self.emcc_args += ['-xc', 'no_alloc.c'] + create_file('test.cpp', r''' + #include + + using emscripten::val; + + int main() { + val::global("console").call("log", val("Hello, world!")); + } + ''') + self.emcc_args += ['-xc++', '--bind'] + # Test that even minimal optimisation level optimises allocations away + # for all those temporary emscripten::val values. + self.emcc_args += ['-O1'] + self.do_runf('test.cpp', 'Hello, world!') From 0510c71dcb7d84c842cdcb1586391ea2fd1f5081 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Thu, 12 Oct 2023 23:53:17 +0100 Subject: [PATCH 10/13] Revert "Move reserved handle check to C++ too" This reverts commit e15a9f7eee0f397e11dbee38b467edbc7fff62db. --- src/embind/emval.js | 4 +++- system/include/emscripten/val.h | 11 ++++------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/embind/emval.js b/src/embind/emval.js index e8967b1e26b3b..9ceef12b52009 100644 --- a/src/embind/emval.js +++ b/src/embind/emval.js @@ -80,7 +80,9 @@ var LibraryEmVal = { _emval_free__deps: ['$emval_handles'], _emval_free: (handle) => { - emval_handles.free(handle); + if (handle >= emval_handles.reserved) { + emval_handles.free(handle); + } }, _emval_run_destructors__deps: ['_emval_free', '$Emval', '$runDestructors'], diff --git a/system/include/emscripten/val.h b/system/include/emscripten/val.h index fc826ee1b001f..3495e9c9c006c 100644 --- a/system/include/emscripten/val.h +++ b/system/include/emscripten/val.h @@ -38,10 +38,9 @@ void _emval_register_symbol(const char*); enum { _EMVAL_UNDEFINED = 1, - _EMVAL_NULL, - _EMVAL_TRUE, - _EMVAL_FALSE, - _EMVAL_UNRESERVED_START + _EMVAL_NULL = 2, + _EMVAL_TRUE = 3, + _EMVAL_FALSE = 4 }; typedef struct _EM_DESTRUCTORS* EM_DESTRUCTORS; @@ -340,9 +339,7 @@ struct val_metadata { // should be only accessible from dec_ref ~val_metadata() { - if (handle >= EM_VAL(internal::_EMVAL_UNRESERVED_START)) { - internal::_emval_free(handle); - } + _emval_free(handle); } }; From 333caed4e35a4e07195ae6dbcbaabb65efe2172b Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Fri, 13 Oct 2023 00:57:20 +0100 Subject: [PATCH 11/13] Rebaseline sigs --- src/library_sigs.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/library_sigs.js b/src/library_sigs.js index 02c0ef152512f..5e82b4fd256b7 100644 --- a/src/library_sigs.js +++ b/src/library_sigs.js @@ -339,16 +339,15 @@ sigs = { _emval_await__sig: 'pp', _emval_call__sig: 'ppipp', _emval_call_method__sig: 'dppppp', - _emval_decref__sig: 'vp', _emval_delete__sig: 'ipp', _emval_equals__sig: 'ipp', + _emval_free__sig: 'vp', _emval_get_global__sig: 'pp', _emval_get_method_caller__sig: 'pip', _emval_get_module_property__sig: 'pp', _emval_get_property__sig: 'ppp', _emval_greater_than__sig: 'ipp', _emval_in__sig: 'ipp', - _emval_incref__sig: 'vp', _emval_instanceof__sig: 'ipp', _emval_is_number__sig: 'ip', _emval_is_string__sig: 'ip', From 04fd58761a8e57b7e4b4cc896458d5e36fdba744 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Fri, 13 Oct 2023 19:48:07 +0100 Subject: [PATCH 12/13] Bring back move constructor --- system/include/emscripten/val.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/system/include/emscripten/val.h b/system/include/emscripten/val.h index 3495e9c9c006c..f46bba778390d 100644 --- a/system/include/emscripten/val.h +++ b/system/include/emscripten/val.h @@ -442,7 +442,9 @@ class val { : val(internal::_emval_new_cstring(v)) {} - val(const val& v) : data(v.data->inc_ref()) {} + val(val&& v) : data(v.data->inc_ref()) {} + + val(const val& v) : val(std::move(v)) {} ~val() { data->dec_ref(); From 5d7bf615d4c60ff526c9e50e98e11c0a31dd9758 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Fri, 13 Oct 2023 19:48:34 +0100 Subject: [PATCH 13/13] Unwrap reserved values --- src/embind/emval.js | 8 ++++---- src/library.js | 19 +++++++++++-------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/embind/emval.js b/src/embind/emval.js index 9ceef12b52009..e91c95bbfa35e 100644 --- a/src/embind/emval.js +++ b/src/embind/emval.js @@ -27,10 +27,10 @@ var LibraryEmVal = { // reserve some special values. These never get de-allocated. // The HandleAllocator takes care of reserving zero. emval_handles.allocated.push( - {value: undefined}, - {value: null}, - {value: true}, - {value: false}, + undefined, + null, + true, + false, ); emval_handles.reserved = emval_handles.allocated.length Module['count_emval_handles'] = count_emval_handles; diff --git a/src/library.js b/src/library.js index 6b3441d2de369..181c4740cfece 100644 --- a/src/library.js +++ b/src/library.js @@ -3598,16 +3598,21 @@ addToLibrary({ #endif }, + // sentinel for invalid handles; it's intentionally a distinct object + // so that we could distinguish it from `undefined` as an actual stored value + $invalidHandleSentinel: {}, + + $handleAllocatorInit__deps: ['$invalidHandleSentinel'], $handleAllocatorInit: function() { Object.assign(HandleAllocator.prototype, /** @lends {HandleAllocator.prototype} */ { get(id) { #if ASSERTIONS - assert(this.allocated[id] !== undefined, `invalid handle: ${id}`); + assert(this.allocated[id] !== invalidHandleSentinel, `invalid handle: ${id}`); #endif return this.allocated[id]; }, has(id) { - return this.allocated[id] !== undefined; + return this.allocated[id] !== invalidHandleSentinel; }, allocate(handle) { var id = this.freelist.pop() || this.allocated.length; @@ -3616,22 +3621,20 @@ addToLibrary({ }, free(id) { #if ASSERTIONS - assert(this.allocated[id] !== undefined); + assert(this.allocated[id] !== invalidHandleSentinel); #endif - // Set the slot to `undefined` rather than using `delete` here since - // apparently arrays with holes in them can be less efficient. - this.allocated[id] = undefined; + this.allocated[id] = invalidHandleSentinel; this.freelist.push(id); } }); }, $HandleAllocator__postset: 'handleAllocatorInit()', - $HandleAllocator__deps: ['$handleAllocatorInit'], + $HandleAllocator__deps: ['$handleAllocatorInit', '$invalidHandleSentinel'], $HandleAllocator__docs: '/** @constructor */', $HandleAllocator: function() { // Reserve slot 0 so that 0 is always an invalid handle - this.allocated = [undefined]; + this.allocated = [invalidHandleSentinel]; this.freelist = []; },