pmed / v8pp

Bind C++ functions and classes into V8 JavaScript engine
http://pmed.github.io/v8pp/
Other
901 stars 121 forks source link

Destroy external objects in context #147

Closed wflohry closed 3 years ago

wflohry commented 3 years ago

The current v8 library does not provide any guarantees about when garbage collection of external objects will be performed, even after the underlying context is destroyed. This can cause issues with tests (valgrind, asan, etc.) and during runtime (especially for large externally-managed objects).

This PR seeks to the implement basic cleanup method [0] during v8pp::context destruction by visiting all objects created with external_data<T>, which are derived from a new base class external_data_base. This manual destruction incurs a small runtime cost during v8pp::context (including one call to destroyObjects and a vtable lookup for each object) but is likely preferable to calling the GC cleanup directly.

The tables below show the before and after results of a leak check of v8pp_test using valgrind:

Before:

== LEAK SUMMARY:
==    definitely lost: 2,008 bytes in 59 blocks
==    indirectly lost: 0 bytes in 0 blocks
==      possibly lost: 0 bytes in 0 blocks
==    still reachable: 153 bytes in 4 blocks
==         suppressed: 0 bytes in 0 blocks

After:

== LEAK SUMMARY:
==    definitely lost: 8 bytes in 2 blocks
==    indirectly lost: 0 bytes in 0 blocks
==      possibly lost: 0 bytes in 0 blocks
==    still reachable: 153 bytes in 4 blocks
==         suppressed: 0 bytes in 0 blocks

The 8 bytes in the 'after' leak summary don't seem to be from external_data but rather v8pp::factory<> in test_class.cpp.

[0] https://itnext.io/v8-wrapped-objects-lifecycle-42272de712e0

pmed commented 3 years ago

Hi @wflohry,

thanks for the pull request! I've merged it onto the master, and backported to c++17 branches.

In a9f9c76643df277c8c733fad19e86232a0adf8fb I also hid the external data destroy internals inside of the external_data class, and moved the v8::PersistentHandleVisitor implementation there.