src: track cppgc wrappers with a list in Realm #56534
Conversation
nodejs-github-bot
added
c++
github-actions
bot
removed
the
request-ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56534 +/- ##
==========================================
- Coverage 90.20% 90.19% -0.02%
==========================================
Files 630 632 +2
Lines 185307 185394 +87
Branches 36271 36288 +17
==========================================
+ Hits 167160 167208 +48
- Misses 11085 11140 +55
+ Partials 7062 7046 -16
🚀 New features to boost your workflow:
|
joyeecheung
force-pushed
the
cppgc-list
branch
from
37e7bac to
195a9a4
Compare
github-actions
bot
removed
the
request-ci
joyeecheung
force-pushed
the
cppgc-list
branch
from
195a9a4 to
9b53840
Compare
|
Trying to rebase #51017 on top of it to verify the "use list for heap snapshot integration" idea. It seems we'll need to update the MemoryTracker code to stop creating duplicate nodes for cppgc wrappers (either that, or use something different for the cppgc wrapper list). Still looking into it.. |
joyeecheung
force-pushed
the
cppgc-list
branch
3 times, most recently
from
ed3648a to
036116a
Compare
| ```cpp | ||
| private: | ||
| CPPGC_USING_PRE_FINALIZER(MyWrap, Clean); | ||
| ``` |
There was a problem hiding this comment.
Some of these rules may be non-obvious to many developers. For example, let's say someone has a wrapped object that is holding a BaseObjectPtr<Foo> as a member... Just based on reading this documentation, as a newcomer I would not know which of these conditions may apply. Do I need to clear the BaseObjectPtr in the CleanEnvResource callback? What about if it has a v8::Global<...> member field? Is it ok to just let that be cleared by the default destructor? etc. I think calling out a few more detailed examples (and not partial examples like you have currently) of each case would be most helpful.
There was a problem hiding this comment.
let's say someone has a wrapped object that is holding a
BaseObjectPtr<Foo>as a member
I don't think this would be supported in the nearest future, or will ever be supported - a cppgc-object should not hold on to a BaseObjectPtr<Foo> without converting that BaseObjectPtr<Foo> to cppgc-managed first - and then in that case they should use cppgc::Member<Foo> to hold on to that instead. The migration needs to be done bottom-to-top. In the case of using cppgc::Member<Foo>, nothing special needs to be done in the destructor, V8 will handle that properly.
What about if it has a v8::Global<...> member field?
I think in most cases, this should be v8::TracedReference instead. In that case nothing special needs to be done in the destructor. This has been documented in the Creating C++ to JavaScript references in cppgc-managed objects section.
There was a problem hiding this comment.
Added a note about not supporting cross BaseObject and cppgc-managed wrapeprs. I don't think it's impossible but someone needs to implement some helpers for that first (I am not 100% sure what needs to be done, but using cppgc::Persistent would be a start).
There was a problem hiding this comment.
I wonder if we can somehow have c++ linting detect it for us if someone includes a BaseObjectPtr member in one of these? .... in any case, this all sounds fine, I'd just like to make sure this gets included somehow in the documentation.
joyeecheung
changed the title
joyeecheung
force-pushed
the
cppgc-list
branch
2 times, most recently
from
909a80a to
6fd34b0
Compare
|
I did a few updates to the design:
|
This allows us to perform cleanups of cppgc wrappers that rely on a
living Realm during Realm shutdown. Otherwise the cleanup may happen
during object destruction, which can be triggered by GC after Realm
shutdown, leading to invalid access to Realm.
The general pattern for this type of non-trivial destruction is
designed to be:
```
class MyWrap final : CPPGC_MIXIN(MyWrap) {
public:
~MyWrap() { this->Finalize(); }
void Clean(Realm* realm) override {
// Do cleanup that relies on a living Realm. This would be
// called by CppgcMixin::Finalize() first during Realm
// shutdown, while the Realm is still alive. If the destructor
// calls Finalize() again later during garbage collection that
// happens after Realm shutdown, Clean() would be skipped,
// preventing invalid access to the Realm.
}
}
```
In addition, this allows us to trace external memory held by the
wrappers in the heap snapshots if we add synthethic edges between the
wrappers and other nodes in the embdder graph callback, or to perform
snapshot serialization for them.
github-actions
bot
removed
the
request-ci
src: track cppgc wrappers with a list in Realm
This allows us to perform cleanups of cppgc wrappers that rely
on a living Realm during Realm shutdown. Otherwise
the cleanup may happen during object destruction, which can
be triggered by GC after Realm shutdown, leading to invalid
access to Realm.
The general pattern for this type of non-trivial destruction is
designed to be:
In addition, this allows us to trace external memory held by the wrappers
in the heap snapshots if we add synthethic edges between the wrappers
and other nodes in the embdder graph callback, or to perform snapshot
serialization for them.