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/src/embind/emval.js b/src/embind/emval.js index 52b3d7b7b1e19..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; @@ -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).value; - }, + toValue: (handle) => emval_handles.get(handle), toHandle: (value) => { switch (value) { @@ -77,31 +72,24 @@ 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); } }, - _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'], 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 = []; }, 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', diff --git a/system/include/emscripten/val.h b/system/include/emscripten/val.h index bd35eeff03243..f46bba778390d 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); @@ -287,6 +286,63 @@ 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; + } + + // 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 { + auto ptr = malloc(count * sizeof(val_metadata)); + if (!ptr) abort(); + return ptr; + } + + void operator delete(void* ptr) noexcept { + free(ptr); + } + +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) \ @@ -386,30 +442,16 @@ 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(val&& v) : data(v.data->inc_ref()) {} - val(const val& v) : val(v.as_handle()) { - internal::_emval_incref(handle); - } + val(const val& v) : val(std::move(v)) {} ~val() { - if (handle) { - internal::_emval_decref(as_handle()); - handle = 0; - } + 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) & { @@ -597,7 +639,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 @@ -621,8 +663,7 @@ class val { return v; } - pthread_t thread; - EM_VAL handle; + internal::val_metadata* data; friend struct internal::BindingType; }; @@ -662,9 +703,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(); - _emval_incref(handle); - return handle; + return v.as_handle(); } static val fromWireType(WireType v) { return val::take_ownership(v); 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!')