From 993bc994153ad56d41989b54435b5514d772dcb9 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Tue, 26 Oct 2021 21:46:55 +0100 Subject: [PATCH 01/10] Update Embind+GC integration (#15327) --- src/embind/embind.js | 50 +++++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/src/embind/embind.js b/src/embind/embind.js index dfd09a96ae96c..fc89d41d4c8d7 100644 --- a/src/embind/embind.js +++ b/src/embind/embind.js @@ -17,7 +17,7 @@ /*global typeDependencies, flushPendingDeletes, getTypeName, getBasestPointer, throwBindingError, UnboundTypeError, _embind_repr, registeredInstances, registeredTypes, getShiftFromSize*/ /*global ensureOverloadTable, embind__requireFunction, awaitingDependencies, makeLegalFunctionName, embind_charCodes:true, registerType, createNamedFunction, RegisteredPointer, throwInternalError*/ /*global simpleReadValueFromPointer, floatReadValueFromPointer, integerReadValueFromPointer, enumReadValueFromPointer, replacePublicSymbol, craftInvokerFunction, tupleRegistrations*/ -/*global finalizationGroup, attachFinalizer, detachFinalizer, releaseClassHandle, runDestructor*/ +/*global finalizationRegistry, attachFinalizer, detachFinalizer, releaseClassHandle, runDestructor*/ /*global ClassHandle, makeClassHandle, structRegistrations, whenDependentTypesAreResolved, BindingError, deletionQueue, delayFunction:true, upcastPointer*/ /*global exposePublicSymbol, heap32VectorToArray, new_, RegisteredPointer_getPointee, RegisteredPointer_destructor, RegisteredPointer_deleteObject, char_0, char_9*/ /*global getInheritedInstanceCount, getLiveInheritedInstances, setDelayFunction, InternalError, runDestructors*/ @@ -1740,38 +1740,50 @@ var LibraryEmbind = { } }, - $finalizationGroup: false, + $finalizationRegistry: false, - $detachFinalizer_deps: ['$finalizationGroup'], + $detachFinalizer_deps: ['$finalizationRegistry'], $detachFinalizer: function(handle) {}, - $attachFinalizer__deps: ['$finalizationGroup', '$detachFinalizer', + $attachFinalizer__deps: ['$finalizationRegistry', '$detachFinalizer', '$releaseClassHandle'], $attachFinalizer: function(handle) { - if ('undefined' === typeof FinalizationGroup) { + if ('undefined' === typeof FinalizationRegistry) { attachFinalizer = function (handle) { return handle; }; return handle; } - // If the running environment has a FinalizationGroup (see + // If the running environment has a FinalizationRegistry (see // https://github.com/tc39/proposal-weakrefs), then attach finalizers - // for class handles. We check for the presence of FinalizationGroup + // for class handles. We check for the presence of FinalizationRegistry // at run-time, not build-time. - finalizationGroup = new FinalizationGroup(function (iter) { - for (var result = iter.next(); !result.done; result = iter.next()) { - var $$ = result.value; - if (!$$.ptr) { - console.warn('object already deleted: ' + $$.ptr); - } else { - releaseClassHandle($$); - } - } + finalizationRegistry = new FinalizationRegistry(function (info) { +#if ASSERTIONS + console.warn(info.leakWarning.stack.replace(/^Error: /, '')); +#endif + releaseClassHandle(info.$$); }); attachFinalizer = function(handle) { - finalizationGroup.register(handle, handle.$$, handle.$$); - return handle; + var $$ = handle.$$; + var info = { $$: $$ }; +#if ASSERTIONS + // Create a warning as an Error instance in advance so that we can store + // the current stacktrace and point to it when / if a leak is detected. + // This is more useful than the empty stacktrace of `FinalizationRegistry` + // callback. + var cls = $$.ptrType.registeredClass; + info.leakWarning = new Error("Embind found a leaked C++ instance " + cls.name + " <0x" + $$.ptr.toString(16) + ">.\n" + + "We'll free it automatically in this case, but this functionality is not reliable across various environments.\n" + + "Make sure to invoke .delete() manually once you're done with the instance instead.\n" + + "Originally allocated"); // `.stack` will add "at ..." after this sentence + if ('captureStackTrace' in Error) { + Error.captureStackTrace(info.leakWarning, cls.constructor); + } +#endif + finalizationRegistry.register(handle, info, handle); + return handle; }; detachFinalizer = function(handle) { - finalizationGroup.unregister(handle.$$); + finalizationRegistry.unregister(handle); }; return attachFinalizer(handle); }, From 2aaa1ec8d77c809502a0d2f71a5e9f065c015223 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rton=20Marczell?= Date: Mon, 3 Jan 2022 18:29:01 +0100 Subject: [PATCH 02/10] Finalize smart ptr handles only --- src/embind/embind.js | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/src/embind/embind.js b/src/embind/embind.js index fc89d41d4c8d7..bc3aa2d16c75f 100644 --- a/src/embind/embind.js +++ b/src/embind/embind.js @@ -1758,29 +1758,33 @@ var LibraryEmbind = { // at run-time, not build-time. finalizationRegistry = new FinalizationRegistry(function (info) { #if ASSERTIONS - console.warn(info.leakWarning.stack.replace(/^Error: /, '')); + console.warn(info.leakWarning.stack.replace(/^Error: /, '')); #endif - releaseClassHandle(info.$$); + releaseClassHandle(info.$$); }); attachFinalizer = function(handle) { - var $$ = handle.$$; - var info = { $$: $$ }; + var $$ = handle.$$; + var hasSmartPtr = !!$$.smartPtr; + if (hasSmartPtr) { + // We should not call the destructor on raw pointers in case other code expects the pointee to live + var info = { $$: $$ }; #if ASSERTIONS - // Create a warning as an Error instance in advance so that we can store - // the current stacktrace and point to it when / if a leak is detected. - // This is more useful than the empty stacktrace of `FinalizationRegistry` - // callback. - var cls = $$.ptrType.registeredClass; - info.leakWarning = new Error("Embind found a leaked C++ instance " + cls.name + " <0x" + $$.ptr.toString(16) + ">.\n" + - "We'll free it automatically in this case, but this functionality is not reliable across various environments.\n" + - "Make sure to invoke .delete() manually once you're done with the instance instead.\n" + - "Originally allocated"); // `.stack` will add "at ..." after this sentence - if ('captureStackTrace' in Error) { - Error.captureStackTrace(info.leakWarning, cls.constructor); - } -#endif - finalizationRegistry.register(handle, info, handle); - return handle; + // Create a warning as an Error instance in advance so that we can store + // the current stacktrace and point to it when / if a leak is detected. + // This is more useful than the empty stacktrace of `FinalizationRegistry` + // callback. + var cls = $$.ptrType.registeredClass; + info.leakWarning = new Error("Embind found a leaked C++ instance " + cls.name + " <0x" + $$.ptr.toString(16) + ">.\n" + + "We'll free it automatically in this case, but this functionality is not reliable across various environments.\n" + + "Make sure to invoke .delete() manually once you're done with the instance instead.\n" + + "Originally allocated"); // `.stack` will add "at ..." after this sentence + if ('captureStackTrace' in Error) { + Error.captureStackTrace(info.leakWarning, cls.constructor); + } + #endif + finalizationRegistry.register(handle, info, handle); + } + return handle; }; detachFinalizer = function(handle) { finalizationRegistry.unregister(handle); From 46729fbe4e865cc35db28cbdd5e88fa750fabcca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rton=20Marczell?= Date: Tue, 4 Jan 2022 11:38:58 +0100 Subject: [PATCH 03/10] Add docs --- .../connecting_cpp_and_javascript/embind.rst | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/site/source/docs/porting/connecting_cpp_and_javascript/embind.rst b/site/source/docs/porting/connecting_cpp_and_javascript/embind.rst index da78546d3d36a..25bd522fe311f 100644 --- a/site/source/docs/porting/connecting_cpp_and_javascript/embind.rst +++ b/site/source/docs/porting/connecting_cpp_and_javascript/embind.rst @@ -200,12 +200,14 @@ to enable the closure compiler. Memory management ================= -JavaScript, specifically ECMA-262 Edition 5.1, does not support `finalizers`_ -or weak references with callbacks. Therefore there is no way for Emscripten -to automatically call the destructors on C++ objects. +JavaScript only gained support for `finalizers`_ in ECMAScript 2021, or ECMA-262 +Edition 12. The new API is called `FinalizationRegistry`_ and it still does not +offer any guarantees that the provided finalization callback will be called. +Embind uses this for cleanup if available, but only for smart pointers, +and only as a last resort. -.. warning:: JavaScript code must explicitly delete any C++ object handles - it has received, or the Emscripten heap will grow indefinitely. +.. warning:: It is strongly recommended that JavaScript code explicitly deletes + any C++ object handles it has received. The :js:func:`delete()` JavaScript method is provided to manually signal that a C++ object is no longer needed and can be deleted: @@ -1020,6 +1022,7 @@ real-world applications has proved to be more than acceptable. .. _Connecting C++ and JavaScript on the Web with Embind: http://chadaustin.me/2014/09/connecting-c-and-javascript-on-the-web-with-embind/ .. _Boost.Python: http://www.boost.org/doc/libs/1_56_0/libs/python/doc/ .. _finalizers: http://en.wikipedia.org/wiki/Finalizer +.. _FinalizationRegistry: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry .. _Reference Counting: https://en.wikipedia.org/wiki/Reference_counting .. _Boost.Python-like raw pointer policies: https://wiki.python.org/moin/boost.python/CallPolicy .. _Backbone.js: http://backbonejs.org/#Model-extend From 3f6cb003a7feae2ad357428d033e9580f9acd4d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rton=20Marczell?= Date: Tue, 11 Jan 2022 14:30:44 +0100 Subject: [PATCH 04/10] Restore arrow function usage from merge --- src/embind/embind.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/embind/embind.js b/src/embind/embind.js index 9fcb3c6af1b0f..aadd0c5e4bc68 100644 --- a/src/embind/embind.js +++ b/src/embind/embind.js @@ -1743,7 +1743,7 @@ var LibraryEmbind = { '$releaseClassHandle'], $attachFinalizer: function(handle) { if ('undefined' === typeof FinalizationRegistry) { - attachFinalizer = function (handle) { return handle; }; + attachFinalizer = (handle) => { return handle; }; return handle; } // If the running environment has a FinalizationRegistry (see @@ -1756,7 +1756,7 @@ var LibraryEmbind = { #endif releaseClassHandle(info.$$); }); - attachFinalizer = function(handle) { + attachFinalizer = (handle) => { var $$ = handle.$$; var hasSmartPtr = !!$$.smartPtr; if (hasSmartPtr) { @@ -1780,7 +1780,7 @@ var LibraryEmbind = { } return handle; }; - detachFinalizer = function(handle) { + detachFinalizer = (handle) => { finalizationRegistry.unregister(handle); }; return attachFinalizer(handle); From c1071b7172c9f0e0063cab97495c1ce791664888 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rton=20Marczell?= <72875131+mmarczell-graphisoft@users.noreply.github.com> Date: Mon, 17 Jan 2022 15:50:09 +0100 Subject: [PATCH 05/10] Fix code review suggestion --- src/embind/embind.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/embind/embind.js b/src/embind/embind.js index aadd0c5e4bc68..bde9c978db379 100644 --- a/src/embind/embind.js +++ b/src/embind/embind.js @@ -1743,7 +1743,7 @@ var LibraryEmbind = { '$releaseClassHandle'], $attachFinalizer: function(handle) { if ('undefined' === typeof FinalizationRegistry) { - attachFinalizer = (handle) => { return handle; }; + attachFinalizer = (handle) => handle; return handle; } // If the running environment has a FinalizationRegistry (see From 20cd522b00d0607c372ab5034716d40617fc7456 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=CC=81rton=20Marczell?= Date: Tue, 18 Jan 2022 12:04:39 +0100 Subject: [PATCH 06/10] Improve stack trace generation --- src/embind/embind.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/embind/embind.js b/src/embind/embind.js index aadd0c5e4bc68..99b615dfa7801 100644 --- a/src/embind/embind.js +++ b/src/embind/embind.js @@ -1773,7 +1773,7 @@ var LibraryEmbind = { "Make sure to invoke .delete() manually once you're done with the instance instead.\n" + "Originally allocated"); // `.stack` will add "at ..." after this sentence if ('captureStackTrace' in Error) { - Error.captureStackTrace(info.leakWarning, cls.constructor); + Error.captureStackTrace(info.leakWarning, Object.getPrototypeOf($$.ptrType).fromWireType); } #endif finalizationRegistry.register(handle, info, handle); From 08a0c393eeeab3ee470b27e7e4ecf4920370e780 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=CC=81rton=20Marczell?= Date: Tue, 18 Jan 2022 13:25:12 +0100 Subject: [PATCH 07/10] Simplify stack trace generation --- src/embind/embind.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/embind/embind.js b/src/embind/embind.js index 7a3a6e157b153..9e2c6da3f6d9f 100644 --- a/src/embind/embind.js +++ b/src/embind/embind.js @@ -1740,7 +1740,7 @@ var LibraryEmbind = { $detachFinalizer: function(handle) {}, $attachFinalizer__deps: ['$finalizationRegistry', '$detachFinalizer', - '$releaseClassHandle'], + '$releaseClassHandle', '$RegisteredPointer_fromWireType'], $attachFinalizer: function(handle) { if ('undefined' === typeof FinalizationRegistry) { attachFinalizer = (handle) => handle; @@ -1773,7 +1773,7 @@ var LibraryEmbind = { "Make sure to invoke .delete() manually once you're done with the instance instead.\n" + "Originally allocated"); // `.stack` will add "at ..." after this sentence if ('captureStackTrace' in Error) { - Error.captureStackTrace(info.leakWarning, Object.getPrototypeOf($$.ptrType).fromWireType); + Error.captureStackTrace(info.leakWarning, RegisteredPointer_fromWireType); } #endif finalizationRegistry.register(handle, info, handle); From c553334ab4e9df1a31abd4bbf2decea803323698 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=CC=81rton=20Marczell?= Date: Tue, 25 Jan 2022 14:19:39 +0100 Subject: [PATCH 08/10] Add test + more arrowfunctionification --- src/embind/embind.js | 6 ++---- tests/embind/test_finalization.cpp | 27 +++++++++++++++++++++++++++ tests/embind/test_finalization.js | 7 +++++++ tests/test_other.py | 12 ++++++++++++ 4 files changed, 48 insertions(+), 4 deletions(-) create mode 100644 tests/embind/test_finalization.cpp create mode 100644 tests/embind/test_finalization.js diff --git a/src/embind/embind.js b/src/embind/embind.js index 9e2c6da3f6d9f..f37da92c9931d 100644 --- a/src/embind/embind.js +++ b/src/embind/embind.js @@ -1750,7 +1750,7 @@ var LibraryEmbind = { // https://github.com/tc39/proposal-weakrefs), then attach finalizers // for class handles. We check for the presence of FinalizationRegistry // at run-time, not build-time. - finalizationRegistry = new FinalizationRegistry(function (info) { + finalizationRegistry = new FinalizationRegistry((info) => { #if ASSERTIONS console.warn(info.leakWarning.stack.replace(/^Error: /, '')); #endif @@ -1780,9 +1780,7 @@ var LibraryEmbind = { } return handle; }; - detachFinalizer = (handle) => { - finalizationRegistry.unregister(handle); - }; + detachFinalizer = (handle) => finalizationRegistry.unregister(handle); return attachFinalizer(handle); }, diff --git a/tests/embind/test_finalization.cpp b/tests/embind/test_finalization.cpp new file mode 100644 index 0000000000000..bebaf03533ed5 --- /dev/null +++ b/tests/embind/test_finalization.cpp @@ -0,0 +1,27 @@ +#include +#include +#include + +class Foo { + std::string mName; + +public: + Foo(std::string name = "Constructed from JS") : mName(name) {} + ~Foo() { std::cout << mName << " destructed" << std::endl; } +}; + +std::shared_ptr foo() { + return std::make_shared("Constructed from C++"); +} + +Foo* pFoo() { return new Foo("Foo*"); } + +using namespace emscripten; + +EMSCRIPTEN_BINDINGS(Marci) { + class_("Foo").smart_ptr_constructor>( + "Foo", std::make_shared); + + function("foo", foo); + function("pFoo", pFoo, allow_raw_pointers()); +} diff --git a/tests/embind/test_finalization.js b/tests/embind/test_finalization.js new file mode 100644 index 0000000000000..4dbe2c6ea6df1 --- /dev/null +++ b/tests/embind/test_finalization.js @@ -0,0 +1,7 @@ +Module.onRuntimeInitialized = () => { + const foo1 = new Module.Foo(); + const foo2 = Module.foo(); + const foo3 = Module.pFoo(); +} + +setTimeout(gc, 100); diff --git a/tests/test_other.py b/tests/test_other.py index 4aa870cafd114..de295ddab2c13 100644 --- a/tests/test_other.py +++ b/tests/test_other.py @@ -2417,6 +2417,18 @@ def test_embind(self): output = self.run_js('a.out.js') self.assertNotContained('FAIL', output) + def test_embind_finalization(self): + self.run_process( + [EMXX, + test_file('embind/test_finalization.cpp'), + '--post-js', test_file('embind/test_finalization.js'), + '--bind'] + ) + self.node_args += ['--expose-gc'] + output = self.run_js('a.out.js', engine=config.NODE_JS) + self.assertContained('Constructed from C++ destructed', output) + self.assertContained('Constructed from JS destructed', output) + def test_emconfig(self): output = self.run_process([emconfig, 'LLVM_ROOT'], stdout=PIPE).stdout.strip() self.assertEqual(output, config.LLVM_ROOT) From 0af199681ea238ff6e424f48e156ae26663755a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=CC=81rton=20Marczell?= Date: Tue, 25 Jan 2022 14:30:24 +0100 Subject: [PATCH 09/10] Improve test with raw pointer exclusion --- tests/test_other.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_other.py b/tests/test_other.py index de295ddab2c13..fae562a90d9a2 100644 --- a/tests/test_other.py +++ b/tests/test_other.py @@ -2428,6 +2428,7 @@ def test_embind_finalization(self): output = self.run_js('a.out.js', engine=config.NODE_JS) self.assertContained('Constructed from C++ destructed', output) self.assertContained('Constructed from JS destructed', output) + self.assertNotContained('Foo* destructed', output) def test_emconfig(self): output = self.run_process([emconfig, 'LLVM_ROOT'], stdout=PIPE).stdout.strip() From 3773ae2f0dfebd7d800f0de17a0d6d722e6f3e67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=CC=81rton=20Marczell?= Date: Tue, 25 Jan 2022 14:41:15 +0100 Subject: [PATCH 10/10] Move test object naming out of default value --- tests/embind/test_finalization.cpp | 4 ++-- tests/embind/test_finalization.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/embind/test_finalization.cpp b/tests/embind/test_finalization.cpp index bebaf03533ed5..a0a3dc42792ad 100644 --- a/tests/embind/test_finalization.cpp +++ b/tests/embind/test_finalization.cpp @@ -6,7 +6,7 @@ class Foo { std::string mName; public: - Foo(std::string name = "Constructed from JS") : mName(name) {} + Foo(std::string name) : mName(name) {} ~Foo() { std::cout << mName << " destructed" << std::endl; } }; @@ -20,7 +20,7 @@ using namespace emscripten; EMSCRIPTEN_BINDINGS(Marci) { class_("Foo").smart_ptr_constructor>( - "Foo", std::make_shared); + "Foo", &std::make_shared); function("foo", foo); function("pFoo", pFoo, allow_raw_pointers()); diff --git a/tests/embind/test_finalization.js b/tests/embind/test_finalization.js index 4dbe2c6ea6df1..52b852ef12339 100644 --- a/tests/embind/test_finalization.js +++ b/tests/embind/test_finalization.js @@ -1,5 +1,5 @@ Module.onRuntimeInitialized = () => { - const foo1 = new Module.Foo(); + const foo1 = new Module.Foo("Constructed from JS"); const foo2 = Module.foo(); const foo3 = Module.pFoo(); }