src: add proper MemoryInfoName to wrappers - #21939
Conversation
Sorry, something went wrong.
|
@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/338/pipeline |
Sorry, something went wrong.
- Use camel case names for memory retainers inherited from AsyncWrap instead of their provider names (which are all in upper case) - Assign class names to wraps so that they appear in the heap snapshot as nodes with class names as node names. Previously some nodes are named with reference names, which are supposed to be edge names instead.
|
Hm... I think I'd prefer it if we could stick to one naming scheme for diagnostic information we provide for AsyncWraps. I do prefer CamelCase like you introduce it here, and I think we should have also been providing CamelCase in async_hooks from the very beginning. I'm not sure we can still make that switch? /cc @nodejs/async_hooks |
Sorry, something went wrong.
|
@addaleax We have more wraps than just the async wraps though, if we want to stick to the naming scheme we will end up polluting all the names including those that do not have async resource provider equivalents and types of different levels, e.g. QueryNaptrWrap v.s. QUERYWRAP. Also for tooling, it's easy to convert camel cases to upper cases, but not the way around. For the async hooks the ship may have already sailed since we have documented those types for quite some time? For heap snapshots the names are mainly used for aggregation, the users may still end up going back to the source so I don't think there is a need to document them down. |
Sorry, something went wrong.
|
just out of curiosity, do any of these not match the class name? maybe you could set it up with like typeid(this).name() or something |
Sorry, something went wrong.
|
@devsnek Some of them do, e.g. TCPWrap is divided into TCPSocketWrap and TCPServerWrap mainly because they are also differentiated as async resources. We could probably make this the default behavior of BaseObject, although RTTI (in non-tests) is advised against in Google's style guide and V8 always avoid using that by explicitly using macros (which is also done in this PR) |
Sorry, something went wrong.
addaleax
left a comment
There was a problem hiding this comment.
Okay, makes sense -- I still think it would be great to eventually have consistency between this and async_hooks. It's not just casing that's different currently, though (e.g. ZLIB vs ZCtx)...
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
- Use camel case names for memory retainers inherited from AsyncWrap instead of their provider names (which are all in upper case) - Assign class names to wraps so that they appear in the heap snapshot as nodes with class names as node names. Previously some nodes are named with reference names, which are supposed to be edge names instead. PR-URL: #21939 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
- Use camel case names for memory retainers inherited from AsyncWrap instead of their provider names (which are all in upper case) - Assign class names to wraps so that they appear in the heap snapshot as nodes with class names as node names. Previously some nodes are named with reference names, which are supposed to be edge names instead. PR-URL: #21939 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
instead of their provider names (which are all in upper case)
as nodes with class names as node names. Previously some nodes are
named with reference names, which are supposed to be edge names
instead.
Before (in DevTools):
After:
Checklist