src: implement MemoryRetainer in Environment by joyeecheung · Pull Request #27018 · nodejs/node

joyeecheung · 2019-03-30T19:42:54Z

This allows us to track the essentially-global objects in
Environment in the heap snapshot. Note that this patch only
tracks the fields that can be tracked correctly. There are
still several types of fields that cannot be tracked:

v8::Data including v8::Private, v8::ObjectTemplate etc.
Internal types that do not implement MemoryRetainer yet
STL containers with MemoryRetainer* inside
STL containers with numeric types inside that should not have their
nodes elided e.g. numeric keys in maps.

The BaseObjects are now no longer globals. They are tracked
as arguments in CleanupHookCallbacks referenced by the Environment
node. This model is closer to how their lifetime is managed
internally.

To track the per-environment strong persistent properties, this patch
divides them into those that are also v8::Value and those that
are just v8::Data. The values can be tracked by the current
memory tracker while the data cannot.

This patch also implements the MemoryRetainer interface in several
internal classes so that they can be tracked in the heap snapshot.

Comparison against the heap snapshot generated before this patch

The base objects are now tracked like this (instead of being globals on their own)

Refs: #26776

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2019-03-30T19:42:56Z

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/3112/pipeline

joyeecheung · 2019-03-30T19:47:43Z

cc @addaleax

joyeecheung · 2019-03-30T19:49:29Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22042/

nodejs-github-bot · 2019-03-30T19:49:47Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22042/

This allows us to track the essentially-global objects in Environment in the heap snapshot. Note that this patch only tracks the fields that can be tracked correctly. There are still several types of fields that cannot be tracked: - v8::Data including v8::Private, v8::ObjectTemplate etc. - Internal types that do not implement MemoryRetainer yet - STL containers with MemoryRetainer* inside - STL containers with numeric types inside that should not have their nodes elided e.g. numeric keys in maps. The `BaseObject`s are now no longer globals. They are tracked as arguments in CleanupHookCallbacks referenced by the Environment node. This model is closer to how their lifetime is managed internally. To track the per-environment strong persistent properties, this patch divides them into those that are also `v8::Value` and those that are just `v8::Data`. The values can be tracked by the current memory tracker while the data cannot. This patch also implements the `MemoryRetainer` interface in several internal classes so that they can be tracked in the heap snapshot.

nodejs-github-bot · 2019-03-31T01:10:29Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22047/

nodejs-github-bot · 2019-03-31T01:12:48Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22048/

joyeecheung · 2019-03-31T01:15:53Z

I added an overload of MemoryTracker::TrackField for CleanupHookCllback - although that requires us to hoist CleanupHookCllback out of Environment for forward declaration. BTW I realized that BaseObject currently inherits from MemoryRetainer and that is probably a bigger source of overhead. I also left a FIXME for that.

@addaleax PTAL, thanks!

bnoordhuis · 2019-04-02T01:16:35Z

+
+ // We keep track of the insertion order for these objects, so that we can
+ // call the callbacks in reverse order when we are cleaning up.
+ uint64_t insertion_order_counter_;


Maybe make these fields const?

This is copied from the previous implementation so I'd prefer to leave this to another patch..

nodejs-github-bot · 2019-04-02T04:04:42Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22111/

joyeecheung · 2019-04-02T09:31:41Z

@bnoordhuis Thanks for the reivews, PTAL

joyeecheung · 2019-04-03T21:13:56Z

Ping @bnoordhuis

If there are no more reviews, I plan to land this later today.

joyeecheung · 2019-04-04T03:09:56Z

Landed in f59ec2a

This allows us to track the essentially-global objects in Environment in the heap snapshot. Note that this patch only tracks the fields that can be tracked correctly. There are still several types of fields that cannot be tracked: - v8::Data including v8::Private, v8::ObjectTemplate etc. - Internal types that do not implement MemoryRetainer yet - STL containers with MemoryRetainer* inside - STL containers with numeric types inside that should not have their nodes elided e.g. numeric keys in maps. The `BaseObject`s are now no longer globals. They are tracked as arguments in CleanupHookCallbacks referenced by the Environment node. This model is closer to how their lifetime is managed internally. To track the per-environment strong persistent properties, this patch divides them into those that are also `v8::Value` and those that are just `v8::Data`. The values can be tracked by the current memory tracker while the data cannot. This patch also implements the `MemoryRetainer` interface in several internal classes so that they can be tracked in the heap snapshot. PR-URL: #27018 Refs: #26776 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Since f59ec2a, `BaseObject` instances were tracked in heap snapshots through their associated `CleanupHookCallback`s which were stored on the `Environment`; however, this is inaccurate, because: - Edges in heap dumps imply a keeps-alive relationship, but cleanup hooks do not keep the `BaseObject`s that they point to alive. - It loses information about whether `BaseObject` instances are GC roots: Even weak `BaseObject`s are now, practically speaking, showing up as hanging off a GC root when that isn’t actually the case (e.g. in the description of nodejs#33468). Thus, this is a partial revert of f59ec2a. Refs: nodejs#33468 Refs: nodejs#27018

Since f59ec2a, `BaseObject` instances were tracked in heap snapshots through their associated `CleanupHookCallback`s which were stored on the `Environment`; however, this is inaccurate, because: - Edges in heap dumps imply a keeps-alive relationship, but cleanup hooks do not keep the `BaseObject`s that they point to alive. - It loses information about whether `BaseObject` instances are GC roots: Even weak `BaseObject`s are now, practically speaking, showing up as hanging off a GC root when that isn’t actually the case (e.g. in the description of #33468). Thus, this is a partial revert of f59ec2a. Refs: #33468 Refs: #27018 PR-URL: #33809 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 30, 2019

addaleax approved these changes Mar 30, 2019
View reviewed changes

Comment threadsrc/env.cc Outdated
Comment threadsrc/env.h Outdated

joyeecheung force-pushed the track-fields branch from 462752e to a5e512cCompare March 31, 2019 01:09

fixup! src: implement MemoryRetainer in Environment
81203f6

joyeecheung force-pushed the track-fields branch from a5e512c to 81203f6Compare March 31, 2019 01:12

jasnell approved these changes Mar 31, 2019
View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 1, 2019

bnoordhuis reviewed Apr 2, 2019
View reviewed changes

fixup! fixup! src: implement MemoryRetainer in Environment
046109e

joyeecheung closed this Apr 4, 2019

addaleax mentioned this pull request Jun 9, 2020
src: do not track BaseObjects via cleanup hooks #33809
Closed
3 tasks

Uh oh!

Uh oh!

Conversation

joyeecheung commented Mar 30, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Mar 30, 2019

Uh oh!

joyeecheung commented Mar 30, 2019

Uh oh!

joyeecheung commented Mar 30, 2019

Uh oh!

nodejs-github-bot commented Mar 30, 2019

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 31, 2019

Uh oh!

nodejs-github-bot commented Mar 31, 2019

Uh oh!

joyeecheung commented Mar 31, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnoordhuisApr 2, 2019

Choose a reason for hiding this comment

Uh oh!

joyeecheungApr 2, 2019

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 2, 2019

Uh oh!

joyeecheung commented Apr 2, 2019

Uh oh!

joyeecheung commented Apr 3, 2019

Uh oh!

joyeecheung commented Apr 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

joyeecheung commented Mar 30, 2019•
edited
Loading

joyeecheung commented Mar 31, 2019•
edited
Loading