Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Leak checker #8687

Open
wants to merge 5 commits into
base: incoming
from

Conversation

Projects
None yet
2 participants
@wingo
Copy link
Contributor

commented May 28, 2019

This PR builds on #8474, adding support for leak-checking instead of automatic finalization.

wingo added some commits Apr 23, 2019

embind: Attach finalizers to class handles if FinalizationGroup is av…
…ailable

FinalizationGroup is part of the WeakRef proposal for JavaScript, which
is currently in "stage 2" of the standardization process:

  https://github.com/tc39/proposal-weakrefs

This patch modifies Embind to automatically attach JS finalizers that
will drop references to C++ objects held by JavaScript objects when
those objects go away, and invoke destructors if a C++ object is no
longer held by JS at all.

Currently the only browser that implements the WeakRef proposal is
Chromium in Git.  To run it with weak refs, run as:

  ./chrome --js-flags=--harmony-weak-refs

(runDestructor): Modify to take $$ pointer as arg, instead of wrapper
handle.  This is needed because we register a finalizer on the wrapper,
but when the finalizer runs we don't have the wrapper any more: just the
$$ pointer.
(releaseClassHandle): Factor out this piece from ClassHandle_delete,
implementing the guts of what's needed by a finalizer.
(finalizationGroup, removeFinalizer, attachFinalizer): Add
implementation of finalizers.
(ClassHandle.delete): Remove finalizer when invoked explicitly.
(makeClassHandle, ClassHandle.clone, ClassHandle.__construct): Register
finalizers on wrappers, using the $$ pointer as the handle given the the
finalizer and the key to unregister the object with the
FinalizationGroup.
Add -s EMBIND_LEAKCHECK=1 support
* src/embind/embind.js: If the user passes -s EMBIND_LEAKCHECK=1, warn
  instead of silently calling an object's destructor if it becomes
  collectable but wasn't explicitly destroy()'d by the user.
* src/settings.js: Add EMBIND_LEAKCHECK=1 variable.
@kripken

This comment has been minimized.

Copy link
Member

commented May 29, 2019

What is the benefit of finding leaks, instead of enabling the automatic cleanup?

@kripken kripken requested a review from chadaustin May 29, 2019

@wingo

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

@kripken The use case is where you are writing both the C/C++ and JS sides of an app and you want to manually manage memory on the JS side. But, it's hard to be sure that you've called .destroy() everywhere that you need to be doing it. So, you use the finalizer support as a leak-checker -- if a JS object becomes finalizable and its associated Wasm memory hasn't been released, we want to know about it so that we can add appropriate .destroy() calls. The logged object includes some information about the object's type, so the developer can inspect the console to see where to look.

For me it's not necessarily leak-check versus automatic cleanup -- to my mind when you enable leak-checking it's a developer tool, not production, so whether to to actually invoke the finalizer or not doesn't matter a whole lot. I think it could make sense to run the finalizer in this case, no problem.

As another data point for this use case:

https://www.hboehm.info/gc/leak.html

Not quite the same but very similar.

@kripken

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

I see, thanks @wingo - that does sound useful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.