★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/21939Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

Closed
wants to merge 1 commit into from

Conversation

Reviewers

@jasnell jasnell

@addaleax addaleax

@cjihrig cjihrig

@tniessen tniessen

@devsnek devsnek

Assignees
No one assigned
Labels
C++ lib / src memory
Projects
None yet
Milestone
No milestone
7 participants
@joyeecheung @nodejs-github-bot @addaleax @devsnek @jasnell @cjihrig @tniessen
@joyeecheung
Copy link
Member

joyeecheung commented Jul 23, 2018 •
edited

  • 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.

Before (in DevTools):
screen shot 2018-07-22 at 9 13 32 am

After:
screen shot 2018-07-22 at 9 11 46 am

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added C++ lib / src labels Jul 23, 2018

@joyeecheung
src: add proper MemoryInfoName to wrappers 
- 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.
Verified
This commit was signed with a verified signature.
@joyeecheung joyeecheung Joyee Cheung
GPG key ID: 92B78A53C8303B8D Learn about signing commits
Loading status checks…
ad10f76

@joyeecheung joyeecheung force-pushed the joyeecheung:wrap-node-name branch from b55b2bc to ad10f76 Jul 23, 2018

@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member Author

joyeecheung commented Jul 23, 2018

cc @addaleax

@addaleax addaleax added the memory label Jul 23, 2018

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member

addaleax commented Jul 23, 2018

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
@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member Author

joyeecheung commented Jul 23, 2018 •
edited

@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.
@devsnek

This comment has been minimized.

Sign in to view
Copy link
Member

devsnek commented Jul 23, 2018

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
@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member Author

joyeecheung commented Jul 23, 2018 •
edited

@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)
@addaleax

addaleax approved these changes Jul 23, 2018
View changes

Copy link
Member

addaleax left a 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)...
@cjihrig

cjihrig approved these changes Jul 23, 2018
View changes

@devsnek

devsnek approved these changes Jul 23, 2018
View changes

@jasnell

jasnell approved these changes Jul 23, 2018
View changes

@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member Author

joyeecheung commented Jul 24, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/15988/
@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member Author

joyeecheung commented Jul 25, 2018

Another CI: https://ci.nodejs.org/job/node-test-pull-request/15994/
@tniessen

tniessen approved these changes Jul 25, 2018
View changes

@joyeecheung joyeecheung referenced this pull request Jul 26, 2018

Closed

src: add trace points to dns #21840

1 of 1 task complete
@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member Author

joyeecheung commented Jul 27, 2018

Landed in 28a3e28, thanks!

@joyeecheung joyeecheung closed this Jul 27, 2018

joyeecheung added a commit that referenced this pull request Jul 27, 2018

@joyeecheung
src: add proper MemoryInfoName to wrappers 
- 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>
Verified
This commit was signed with a verified signature.
@joyeecheung joyeecheung Joyee Cheung
GPG key ID: 92B78A53C8303B8D Learn about signing commits
Loading status checks…
28a3e28

targos added a commit that referenced this pull request Jul 31, 2018

@joyeecheung @targos
src: add proper MemoryInfoName to wrappers 
- 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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
80b5c91

@targos targos referenced this pull request Jul 31, 2018

Merged

v10.8.0 proposal #22040

This was referenced Aug 1, 2018

Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.