From 8f28ba09f2c2ab86e50a06665aa3d95c71bbe570 Mon Sep 17 00:00:00 2001 From: Mike Rolig Date: Thu, 8 Feb 2024 13:09:33 -0800 Subject: [PATCH] Move all Emval liftime tracking to C++. --- src/embind/embind.js | 6 +-- src/embind/emval.js | 73 ++++++++++++--------------------- src/library.js | 16 ++++---- src/library_sigs.js | 4 +- system/include/emscripten/val.h | 59 +++++++++++++++++--------- 5 files changed, 81 insertions(+), 77 deletions(-) diff --git a/src/embind/embind.js b/src/embind/embind.js index bee7627b4c9a0..621a60593a98e 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..aa9728192898e 100644 --- a/src/embind/emval.js +++ b/src/embind/emval.js @@ -12,43 +12,37 @@ /*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; 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.allocated.length === {{{ 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.allocated.length - {{{ EMVAL_RESERVED_HANDLES }}} + - emval_handles.freelist.length; }, _emval_register_symbol__deps: ['$emval_symbols', '$readLatin1String'], @@ -65,58 +59,45 @@ 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) => { + return 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_LAST_RESERVED_HANDLE }}}) { + 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 88cc447b59859..2744ae43500b4 100644 --- a/src/library.js +++ b/src/library.js @@ -3554,22 +3554,26 @@ addToLibrary({ #endif }, + // Sentinel for invalid handles; it's intentionally a distinct object so that + // we can distinguish it from `undefined` as an actual stored value. + $invalidHandleSentinel: {}, + $HandleAllocator__deps: ['$invalidHandleSentinel'], $HandleAllocator: class { constructor() { // TODO(sbc): Use class fields once we allow/enable es2022 in // JavaScript input to acorn and closure. // Reserve slot 0 so that 0 is always an invalid handle - this.allocated = [undefined]; + this.allocated = [invalidHandleSentinel]; this.freelist = []; } 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; @@ -3578,11 +3582,9 @@ 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); }; }, diff --git a/src/library_sigs.js b/src/library_sigs.js index d7d796d6c2102..74bb859a30a17 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..08165a3c9bdca 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,39 @@ 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. + // Move constructor takes over the other item's place in the list for this handle. val(val&& v) : handle(v.handle), thread(v.thread) { v.handle = 0; + if (v.next == &v) { + next = prev = this; + } else { + next = v.next; + prev = v.prev; + next->prev = prev->next = this; + } } - 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) : handle(v.handle), + next(v.next), + prev(const_cast(&v)) , + thread (v.thread) { + const_cast(v).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,9 +644,9 @@ class val { #endif private: - // takes ownership, assumes handle already incref'd and lives on the same thread + // takes ownership, assumes handle lives on the same thread with no other C++ references explicit val(EM_VAL handle) - : handle(handle), thread(pthread_self()) + : handle(handle), next(this), prev(this), thread(pthread_self()) {} template @@ -657,8 +676,11 @@ class val { return v; } - pthread_t thread; EM_VAL handle; + // Circular doubly-linked list of all references to this handle. + val* next; + val* 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));