-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Track Environment fields in heap snapshot? #26776
Comments
cc @nodejs/embedders |
PR-URL: #26824 Refs: #26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #26824 Refs: #26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #26824 Refs: #26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #26824 Refs: #26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #26824 Refs: #26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #26824 Refs: #26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#26824 Refs: nodejs#26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#26824 Refs: nodejs#26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#26824 Refs: nodejs#26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#26824 Refs: nodejs#26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#26824 Refs: nodejs#26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#26824 Refs: nodejs#26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #26824 Refs: #26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #26824 Refs: #26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #26824 Refs: #26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #26824 Refs: #26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #26824 Refs: #26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #26824 Refs: #26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
This allows us to track the essentially-global objects in Environment in the heap snapshot. Note that this patch only tracks the fields that can be tracked correctly. There are still several types of fields that cannot be tracked: - v8::Data including v8::Private, v8::ObjectTemplate etc. - Internal types that do not implement MemoryRetainer yet - STL containers with MemoryRetainer* inside - STL containers with numeric types inside that should not have their nodes elided e.g. numeric keys in maps. The `BaseObject`s are now no longer globals. They are tracked as arguments in CleanupHookCallbacks referenced by the Environment node. This model is closer to how their lifetime is managed internally. To track the per-environment strong persistent properties, this patch divides them into those that are also `v8::Value` and those that are just `v8::Data`. The values can be tracked by the current memory tracker while the data cannot. This patch also implements the `MemoryRetainer` interface in several internal classes so that they can be tracked in the heap snapshot. PR-URL: #27018 Refs: #26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
This allows us to track the essentially-global objects in Environment in the heap snapshot. Note that this patch only tracks the fields that can be tracked correctly. There are still several types of fields that cannot be tracked: - v8::Data including v8::Private, v8::ObjectTemplate etc. - Internal types that do not implement MemoryRetainer yet - STL containers with MemoryRetainer* inside - STL containers with numeric types inside that should not have their nodes elided e.g. numeric keys in maps. The `BaseObject`s are now no longer globals. They are tracked as arguments in CleanupHookCallbacks referenced by the Environment node. This model is closer to how their lifetime is managed internally. To track the per-environment strong persistent properties, this patch divides them into those that are also `v8::Value` and those that are just `v8::Data`. The values can be tracked by the current memory tracker while the data cannot. This patch also implements the `MemoryRetainer` interface in several internal classes so that they can be tracked in the heap snapshot. PR-URL: #27018 Refs: #26776 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
@joyeecheung - will this enable a debugger (like llnode) walk through the reference chains of C++ objects, starting with a JS object? the use case is debugging native memory leaks, that has JS objects as anchors. |
@gireeshpunathil I don't think it's going to be useful for llnode, but it adds more information into the heap snapshots (essentially allowing the heap snapshots to track C++ memory as well) |
At the moment, the Environment fields are not tracked by the heap snapshot (unless they are referenced by some other objects that implements the
MemoryRetainer
interface) - for example, you can't see any of theAliasedBuffer
in the Environment when looking at a heap snapshot taken after bootstrap. Considering the amount of things we attach to the Environment, it should be pretty useful to track those fields in the heap snapshot instead of keeping them invisible for no particular reason.I am thinking about having Environment implmement
MemoryRetainer
, are there any concerns around having it inherit from an abstract class? (considering this is semi-exposed to embedders).The text was updated successfully, but these errors were encountered: