From 14c97538d17e799d9449803f08c6f2deff3c840a Mon Sep 17 00:00:00 2001 From: Mike Rolig Date: Thu, 8 Feb 2024 13:09:33 -0800 Subject: [PATCH 1/7] 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 3275d29c6bf9d..6ac4cfee966bb 100644 --- a/src/library.js +++ b/src/library.js @@ -3548,22 +3548,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; @@ -3572,11 +3576,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 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..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)); From be79be3c0c40c7971c2060c40674e7eb43e82980 Mon Sep 17 00:00:00 2001 From: Mike Rolig Date: Thu, 8 Feb 2024 13:20:16 -0800 Subject: [PATCH 2/7] Rebaseline --- test/code_size/embind_val_wasm.json | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/code_size/embind_val_wasm.json b/test/code_size/embind_val_wasm.json index a199731962377..c611977037cb2 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": 7086, - "a.js.gz": 3000, - "a.wasm": 11433, - "a.wasm.gz": 5725, - "total": 19192, - "total_gz": 9156 + "a.js": 7202, + "a.js.gz": 3031, + "a.wasm": 11498, + "a.wasm.gz": 5766, + "total": 19373, + "total_gz": 9228 } From 7f43b97ab1dce157f7041b71e4a1abea7ab37dc9 Mon Sep 17 00:00:00 2001 From: Mike Rolig Date: Thu, 8 Feb 2024 15:21:41 -0800 Subject: [PATCH 3/7] Flatten arrow function to have no block. --- src/embind/emval.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/embind/emval.js b/src/embind/emval.js index aa9728192898e..39e6b25e7ae70 100644 --- a/src/embind/emval.js +++ b/src/embind/emval.js @@ -82,9 +82,7 @@ var LibraryEmVal = { }, _emval_clone__deps: ['$emval_handles'], - _emval_clone: (handle) => { - return Emval.toHandle(emval_handles.get(handle)); - }, + _emval_clone: (handle) => Emval.toHandle(emval_handles.get(handle)), _emval_free__deps: ['$emval_handles'], _emval_free: (handle) => { From 72f13f675a46c8d50599347ba05d1a31ea95c88e Mon Sep 17 00:00:00 2001 From: Mike Rolig Date: Thu, 15 Feb 2024 13:25:59 -0800 Subject: [PATCH 4/7] Add count() method to HandleAllocator. Use it for count_emval_handles() and for test_promise. --- src/embind/emval.js | 10 ++++------ src/library.js | 19 ++++++++++--------- test/core/test_promise.c | 2 +- 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/embind/emval.js b/src/embind/emval.js index 39e6b25e7ae70..61c9b6dca5ea7 100644 --- a/src/embind/emval.js +++ b/src/embind/emval.js @@ -18,8 +18,7 @@ // 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 = { @@ -34,15 +33,14 @@ var LibraryEmVal = { // The HandleAllocator takes care of reserving zero. emval_handles.allocated.push(undefined, null, true, false); #if ASSERTIONS - assert(emval_handles.allocated.length === {{{ EMVAL_RESERVED_HANDLES }}}); + assert(emval_handles.count() === {{{ EMVAL_RESERVED_HANDLES }}}); #endif Module['count_emval_handles'] = count_emval_handles; }, $count_emval_handles__deps: ['$emval_handles'], $count_emval_handles: () => { - return emval_handles.allocated.length - {{{ EMVAL_RESERVED_HANDLES }}} - - emval_handles.freelist.length; + return emval_handles.count() - {{{ EMVAL_RESERVED_HANDLES }}}; }, _emval_register_symbol__deps: ['$emval_symbols', '$readLatin1String'], @@ -86,7 +84,7 @@ var LibraryEmVal = { _emval_free__deps: ['$emval_handles'], _emval_free: (handle) => { - if (handle > {{{ EMVAL_LAST_RESERVED_HANDLE }}}) { + if (handle > {{{ EMVAL_RESERVED_HANDLES }}}) { emval_handles.free(handle); } }, diff --git a/src/library.js b/src/library.js index 6ac4cfee966bb..4ecfb98732169 100644 --- a/src/library.js +++ b/src/library.js @@ -3548,26 +3548,23 @@ 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. + // Use this allocator object as a placeholder for invalid entries. // Reserve slot 0 so that 0 is always an invalid handle - this.allocated = [invalidHandleSentinel]; + this.allocated = [this]; this.freelist = []; } get(id) { #if ASSERTIONS - assert(this.allocated[id] !== invalidHandleSentinel, `invalid handle: ${id}`); + assert(this.allocated[id] !== this, `invalid handle: ${id}`); #endif return this.allocated[id]; }; has(id) { - return this.allocated[id] !== invalidHandleSentinel; + return this.allocated[id] !== this; }; allocate(handle) { var id = this.freelist.pop() || this.allocated.length; @@ -3576,11 +3573,15 @@ addToLibrary({ }; free(id) { #if ASSERTIONS - assert(this.allocated[id] !== invalidHandleSentinel); + assert(this.allocated[id] !== this); #endif - this.allocated[id] = invalidHandleSentinel; + 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/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 From a73a1b8cf1fbbaae064baffaa02bf81b1b482594 Mon Sep 17 00:00:00 2001 From: Mike Rolig Date: Thu, 15 Feb 2024 23:07:44 -0800 Subject: [PATCH 5/7] Tidy constructors. --- system/include/emscripten/val.h | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/system/include/emscripten/val.h b/system/include/emscripten/val.h index 08165a3c9bdca..72d922f2f5e15 100644 --- a/system/include/emscripten/val.h +++ b/system/include/emscripten/val.h @@ -378,23 +378,21 @@ class val { {} // 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 { + 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; } // 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; + val(const val& v) : val(v.handle, v.thread) { + next = v.next; + prev = const_cast(&v); + prev->next = this; next->prev = this; } @@ -645,9 +643,10 @@ class val { private: // takes ownership, assumes handle lives on the same thread with no other C++ references - explicit val(EM_VAL handle) - : handle(handle), next(this), prev(this), thread(pthread_self()) - {} + 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& ); From 79adbce9c8066bea90f99eefd1084121f6f72be8 Mon Sep 17 00:00:00 2001 From: Mike Rolig Date: Thu, 15 Feb 2024 23:08:18 -0800 Subject: [PATCH 6/7] rebaseline --- test/code_size/embind_val_wasm.json | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/test/code_size/embind_val_wasm.json b/test/code_size/embind_val_wasm.json index c09f6615475f4..c9070513f9ed5 100644 --- a/test/code_size/embind_val_wasm.json +++ b/test/code_size/embind_val_wasm.json @@ -1,11 +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 } From bb2e6625008db855da5f8571f806229f67445421 Mon Sep 17 00:00:00 2001 From: Mike Rolig Date: Sat, 17 Feb 2024 09:05:56 -0800 Subject: [PATCH 7/7] Use mutable keyword rather than const_cast. --- system/include/emscripten/val.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/system/include/emscripten/val.h b/system/include/emscripten/val.h index 72d922f2f5e15..8d3db7f6990f7 100644 --- a/system/include/emscripten/val.h +++ b/system/include/emscripten/val.h @@ -391,7 +391,7 @@ class val { // 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 = const_cast(&v); + prev = &v; prev->next = this; next->prev = this; } @@ -676,9 +676,10 @@ class val { } EM_VAL handle; - // Circular doubly-linked list of all references to this handle. - val* next; - val* prev; + // 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;