diff --git a/src/embind/embind.js b/src/embind/embind.js index 4be33d707b4c3..2f3b085c67999 100644 --- a/src/embind/embind.js +++ b/src/embind/embind.js @@ -9,7 +9,7 @@ /*global _malloc, _free, _memcpy*/ /*global FUNCTION_TABLE, HEAP8, HEAPU8, HEAP16, HEAPU16, HEAP32, HEAPU32, HEAPF32, HEAPF64*/ /*global readLatin1String*/ -/*global Emval, emval_handle_array, __emval_decref*/ +/*global Emval, __emval_free*/ /*jslint sub:true*/ /* The symbols 'fromWireType' and 'toWireType' must be accessed via array notation to be closure-safe since craftInvokerFunction crafts functions as strings that can't be closured. */ // -- jshint doesn't understand library syntax, so we need to specifically tell it about the symbols we define @@ -41,12 +41,12 @@ var LibraryEmbind = { // If register_type is used, emval will be registered multiple times for // different type id's, but only a single type object is needed on the JS side // for all of them. Store the type for reuse. - $EmValType__deps: ['_emval_decref', '$Emval', '$readPointer', '$GenericWireTypeSize'], + $EmValType__deps: ['_emval_free', '$Emval', '$readPointer', '$GenericWireTypeSize'], $EmValType: `{ name: 'emscripten::val', 'fromWireType': (handle) => { var rv = Emval.toValue(handle); - __emval_decref(handle); + __emval_free(handle); return rv; }, 'toWireType': (destructors, value) => Emval.toHandle(value), diff --git a/src/embind/emval.js b/src/embind/emval.js index 8d22ae24839ad..61c9b6dca5ea7 100644 --- a/src/embind/emval.js +++ b/src/embind/emval.js @@ -12,43 +12,35 @@ /*jslint sub:true*/ /* The symbols 'fromWireType' and 'toWireType' must be accessed via array notation to be closure-safe since craftInvokerFunction crafts functions as strings that can't be closured. */ // -- jshint doesn't understand library syntax, so we need to mark the symbols exposed here -/*global getStringOrSymbol, emval_freelist, emval_handles, Emval, __emval_unregister, count_emval_handles, emval_symbols, __emval_decref*/ +/*global getStringOrSymbol, emval_handles, Emval, __emval_unregister, count_emval_handles, emval_symbols, __emval_free*/ /*global emval_addMethodCaller, emval_methodCallers, addToLibrary, global, emval_lookupTypes, makeLegalFunctionName*/ /*global emval_get_global*/ -// Number of handles reserved for non-use (0) or common values w/o refcount. +// Number of handles reserved for non-use (0) or common values never freed. {{{ - globalThis.EMVAL_RESERVED_HANDLES = 5; - globalThis.EMVAL_LAST_RESERVED_HANDLE = globalThis.EMVAL_RESERVED_HANDLES * 2 - 1; + globalThis.EMVAL_RESERVED_HANDLES = 4; null; }}} var LibraryEmVal = { - // Stack of handles available for reuse. - $emval_freelist: [], - // Array of alternating pairs (value, refcount). - $emval_handles: [], + $emval_handles__deps: ['$HandleAllocator'], + $emval_handles: "new HandleAllocator();", $emval_symbols: {}, // address -> string $init_emval__deps: ['$count_emval_handles', '$emval_handles'], $init_emval__postset: 'init_emval();', $init_emval: () => { - // reserve 0 and some special values. These never get de-allocated. - emval_handles.push( - 0, 1, - undefined, 1, - null, 1, - true, 1, - false, 1, - ); + // reserve some special values. These never get de-allocated. + // The HandleAllocator takes care of reserving zero. + emval_handles.allocated.push(undefined, null, true, false); #if ASSERTIONS - assert(emval_handles.length === {{{ EMVAL_RESERVED_HANDLES }}} * 2); + assert(emval_handles.count() === {{{ EMVAL_RESERVED_HANDLES }}}); #endif Module['count_emval_handles'] = count_emval_handles; }, - $count_emval_handles__deps: ['$emval_freelist', '$emval_handles'], + $count_emval_handles__deps: ['$emval_handles'], $count_emval_handles: () => { - return emval_handles.length / 2 - {{{ EMVAL_RESERVED_HANDLES }}} - emval_freelist.length; + return emval_handles.count() - {{{ EMVAL_RESERVED_HANDLES }}}; }, _emval_register_symbol__deps: ['$emval_symbols', '$readLatin1String'], @@ -65,58 +57,43 @@ var LibraryEmVal = { return symbol; }, - $Emval__deps: ['$emval_freelist', '$emval_handles', '$throwBindingError', '$init_emval'], + $Emval__deps: ['$emval_handles', '$throwBindingError', '$init_emval'], $Emval: { toValue: (handle) => { if (!handle) { throwBindingError('Cannot use deleted val. handle = ' + handle); } - #if ASSERTIONS - // handle 2 is supposed to be `undefined`. - assert(handle === 2 || emval_handles[handle] !== undefined && handle % 2 === 0, `invalid handle: ${handle}`); - #endif - return emval_handles[handle]; + return emval_handles.get(handle); }, toHandle: (value) => { switch (value) { - case undefined: return 2; - case null: return 4; - case true: return 6; - case false: return 8; + case undefined: return 1; + case null: return 2; + case true: return 3; + case false: return 4; default:{ - const handle = emval_freelist.pop() || emval_handles.length; - emval_handles[handle] = value; - emval_handles[handle + 1] = 1; - return handle; + return emval_handles.allocate(value); } } } }, - _emval_incref__deps: ['$emval_handles'], - _emval_incref: (handle) => { - if (handle > {{{ EMVAL_LAST_RESERVED_HANDLE }}}) { - emval_handles[handle + 1] += 1; - } - }, + _emval_clone__deps: ['$emval_handles'], + _emval_clone: (handle) => Emval.toHandle(emval_handles.get(handle)), - _emval_decref__deps: ['$emval_freelist', '$emval_handles'], - _emval_decref: (handle) => { - if (handle > {{{ EMVAL_LAST_RESERVED_HANDLE }}} && 0 === --emval_handles[handle + 1]) { - #if ASSERTIONS - assert(emval_handles[handle] !== undefined, `Decref for unallocated handle.`); - #endif - emval_handles[handle] = undefined; - emval_freelist.push(handle); + _emval_free__deps: ['$emval_handles'], + _emval_free: (handle) => { + if (handle > {{{ EMVAL_RESERVED_HANDLES }}}) { + 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 b30e852d66009..db63e9c217940 100644 --- a/src/library.js +++ b/src/library.js @@ -3556,18 +3556,19 @@ addToLibrary({ constructor() { // TODO(sbc): Use class fields once we allow/enable es2022 in // JavaScript input to acorn and closure. + // Use this allocator object as a placeholder for invalid entries. // Reserve slot 0 so that 0 is always an invalid handle - this.allocated = [undefined]; + this.allocated = [this]; this.freelist = []; } get(id) { #if ASSERTIONS - assert(this.allocated[id] !== undefined, `invalid handle: ${id}`); + assert(this.allocated[id] !== this, `invalid handle: ${id}`); #endif return this.allocated[id]; }; has(id) { - return this.allocated[id] !== undefined; + return this.allocated[id] !== this; }; allocate(handle) { var id = this.freelist.pop() || this.allocated.length; @@ -3576,13 +3577,15 @@ addToLibrary({ }; free(id) { #if ASSERTIONS - assert(this.allocated[id] !== undefined); + assert(this.allocated[id] !== this); #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] = this; this.freelist.push(id); }; + count() { + // Handle zero is reserved and always allocated. + return this.allocated.length - this.freelist.length - 1; + } }, $getNativeTypeSize__deps: ['$POINTER_SIZE'], diff --git a/src/library_sigs.js b/src/library_sigs.js index bd173e74558ff..5cb2eafcaf6cb 100644 --- a/src/library_sigs.js +++ b/src/library_sigs.js @@ -342,18 +342,18 @@ sigs = { _emval_await__sig: 'pp', _emval_call__sig: 'dpppp', _emval_call_method__sig: 'dppppp', + _emval_clone__sig: 'pp', _emval_coro_make_promise__sig: 'ppp', _emval_coro_suspend__sig: 'vpp', - _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: 'pipi', _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 81e7c0d2e88f2..8d3db7f6990f7 100644 --- a/system/include/emscripten/val.h +++ b/system/include/emscripten/val.h @@ -46,10 +46,11 @@ extern "C" { void _emval_register_symbol(const char*); enum { - _EMVAL_UNDEFINED = 2, - _EMVAL_NULL = 4, - _EMVAL_TRUE = 6, - _EMVAL_FALSE = 8 + _EMVAL_UNDEFINED = 1, + _EMVAL_NULL = 2, + _EMVAL_TRUE = 3, + _EMVAL_FALSE = 4, + _EMVAL_NUM_RESERVED_HANDLES = 5, }; typedef struct _EM_DESTRUCTORS* EM_DESTRUCTORS; @@ -57,8 +58,8 @@ 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); +EM_VAL _emval_clone(EM_VAL value); +void _emval_free(EM_VAL value); void _emval_run_destructors(EM_DESTRUCTORS handle); @@ -376,21 +377,37 @@ 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) { + // Move constructor takes over the other item's place in the list for this handle. + val(val&& v) : val(v.handle, v.thread) { + if (v.next != &v) { + next = v.next; + prev = v.prev; + next->prev = prev->next = this; + } v.handle = 0; + v.next = v.prev = &v; } - val(const val& v) : val(v.as_handle()) { - internal::_emval_incref(handle); + // Copy constructor inserts this new entry into the list for this handle. + val(const val& v) : val(v.handle, v.thread) { + next = v.next; + prev = &v; + prev->next = this; + next->prev = this; } ~val() { if (handle) { - internal::_emval_decref(as_handle()); + if (this == next) { + // The was the only entry in the list, free the value. + if (handle >= reinterpret_cast(internal::_EMVAL_NUM_RESERVED_HANDLES)) { + internal::_emval_free(as_handle()); + } + } else { + // More references exist, remove this entry from the list. + next->prev = prev; + prev->next = next; + } handle = 0; } } @@ -625,10 +642,11 @@ class val { #endif private: - // takes ownership, assumes handle already incref'd and lives on the same thread - explicit val(EM_VAL handle) - : handle(handle), thread(pthread_self()) - {} + // takes ownership, assumes handle lives on the same thread with no other C++ references + explicit val(EM_VAL handle) : val(handle, pthread_self()) {} + + val(EM_VAL handle, pthread_t thread) + : handle(handle), next(this), prev(this), thread(pthread_self()) {} template friend val internal::wrapped_extend(const std::string& , const val& ); @@ -657,8 +675,12 @@ class val { return v; } - pthread_t thread; EM_VAL handle; + // Circular doubly-linked list of all references to this handle, may + // be mutated by copy constructor of new instance inserting itself. + const val mutable* next; + const val mutable* prev; + pthread_t thread; friend struct internal::BindingType; }; @@ -787,9 +809,8 @@ 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; + // Allocate a new handle that the JS fromWireType will free. + return _emval_clone(v.as_handle()); } static T fromWireType(WireType v) { return T(val::take_ownership(v)); diff --git a/test/code_size/embind_val_wasm.json b/test/code_size/embind_val_wasm.json index 94884bb90b1cf..c9070513f9ed5 100644 --- a/test/code_size/embind_val_wasm.json +++ b/test/code_size/embind_val_wasm.json @@ -1,10 +1,10 @@ { "a.html": 673, "a.html.gz": 431, - "a.js": 7080, - "a.js.gz": 2999, - "a.wasm": 11433, - "a.wasm.gz": 5725, - "total": 19186, - "total_gz": 9155 + "a.js": 7214, + "a.js.gz": 3036, + "a.wasm": 11498, + "a.wasm.gz": 5766, + "total": 19385, + "total_gz": 9233 } diff --git a/test/core/test_promise.c b/test/core/test_promise.c index 3a19875acb052..418ebf7d8b088 100644 --- a/test/core/test_promise.c +++ b/test/core/test_promise.c @@ -503,7 +503,7 @@ static em_promise_result_t finish(void** result, void* data, void* value) { // We should not have leaked any handles. EM_ASM({ - promiseMap.allocated.forEach((p) => assert(typeof p === "undefined", "non-destroyed handle")); + assert(promiseMap.count() === 0, "non-destroyed handle"); }); // Cannot exit directly in a promise callback, since it would end up rejecting