-
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
src: add proper MemoryInfoName to wrappers #21939
Conversation
- 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.
b55b2bc
to
ad10f76
Compare
cc @addaleax |
Hm... I think I'd prefer it if we could stick to one naming scheme for diagnostic information we provide for |
@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. 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. |
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 |
@devsnek Some of them do, e.g. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)...
Landed in 28a3e28, thanks! |
- 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
make -j4 test
(UNIX), orvcbuild test
(Windows) passes