-
Notifications
You must be signed in to change notification settings - Fork 30.7k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
doc: fix the example implementation of MemoryRetainer #26262
Conversation
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
@joyeecheung sadly an error occured when I tried to trigger a build :( |
|
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.
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?
@addaleax #26161 probably does not eliminate the need for careful calculations, for example it does not cover the all types specialized by Maybe it would help if we add a enum/boolean to the |
* 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); |
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.
How about sizeof(*this)
and sizeof(non_pointer_retainer)
?
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.
Ping @joyeecheung
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.
@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
.
Landed in f3d6207 |
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>
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>
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>
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>
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), orvcbuild test
(Windows) passes