Skip to content
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

doc: fix the example implementation of MemoryRetainer #26262

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

We need to be careful not to include the size of non-pointer
fields in the parent's self size if we want to track them separately
as a different node.

Refs: https://github.com/nodejs/node/pull/26161/files#r259170771

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

We need to be careful not to include the size of non-pointer
fields in the parent's self size if we want to track them separately
as a different node.

Refs: https://github.com/nodejs/node/pull/26161/files#r259170771
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung sadly an error occured when I tried to trigger a build :(

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 22, 2019
@joyeecheung
Copy link
Member Author

joyeecheung commented Feb 22, 2019

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I think I’d like something like #26161 for solving this problem for the future, rather than having to take care of the NonPointerRetainerClass field in 2 places?

@joyeecheung
Copy link
Member Author

joyeecheung commented Feb 23, 2019

@addaleax #26161 probably does not eliminate the need for careful calculations, for example it does not cover the all types specialized by TrackField so for example when there is a uv_async_t member the user wants to track separately they still need to take care of this. They also need to be careful to subtract size of the fields they track with TrackFieldWithSize(), which may be more complicated than just subtracting sizeof(FieldType) depending on how much memory they want to shift into the child node.

Maybe it would help if we add a enum/boolean to the TrackField implementations notifying the tracker to subtract sizeof(FieldType) automatically but I am not sure how complicated the calculation would become when we need to call it recursively for STL containers and other composite types.

* return sizeof(ExampleRetainer);
* // We need to exclude the size of non_pointer_retainer so that
* // we can track it separately in ExampleRetainer::MemoryInfo().
* return sizeof(ExampleRetainer) - sizeof(NonPointerRetainerClass);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about sizeof(*this) and sizeof(non_pointer_retainer)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@joyeecheung joyeecheung Mar 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TimothyGu For the example itself there probably is no difference - whatever that makes the calculation easier to understand for the writer is good enough.

One thing to note: if non_pointer_retainer is of certain types, the equation may be more complicated than a - sizeof(non_pointer_retainer) if the self size of that node that needs to be split out of the parent is not exactly sizeof(non_pointer_retainer). So, keeping sizeof(NonPointerRetainerClass) makes that obvious, as in actual code the member name may be as vague as obj or val.

@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 6, 2019

Landed in f3d6207

@joyeecheung joyeecheung closed this Mar 6, 2019
joyeecheung added a commit that referenced this pull request Mar 6, 2019
We need to be careful not to include the size of non-pointer
fields in the parent's self size if we want to track them separately
as a different node.

Refs: https://github.com/nodejs/node/pull/26161/files#r259170771

PR-URL: #26262
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
We need to be careful not to include the size of non-pointer
fields in the parent's self size if we want to track them separately
as a different node.

Refs: https://github.com/nodejs/node/pull/26161/files#r259170771

PR-URL: nodejs#26262
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
We need to be careful not to include the size of non-pointer
fields in the parent's self size if we want to track them separately
as a different node.

Refs: https://github.com/nodejs/node/pull/26161/files#r259170771

PR-URL: #26262
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 16, 2019
We need to be careful not to include the size of non-pointer
fields in the parent's self size if we want to track them separately
as a different node.

Refs: https://github.com/nodejs/node/pull/26161/files#r259170771

PR-URL: #26262
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants