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

src: implement MemoryRetainer in Environment #27018

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Mar 30, 2019

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

Screen Shot 2019-03-30 at 3 01 20 PM

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

Screen Shot 2019-03-30 at 3 45 34 PM

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 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
@joyeecheung
Copy link
Member Author

cc @addaleax

@joyeecheung
Copy link
Member Author

1 similar comment
@nodejs-github-bot
Copy link
Collaborator

src/env.cc Show resolved Hide resolved
src/env.h Outdated Show resolved Hide resolved
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
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 31, 2019

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!

@BridgeAR 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
src/env.cc Show resolved Hide resolved
src/env.h Show resolved Hide resolved

// 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_;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe make these fields const?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

@bnoordhuis Thanks for the reivews, PTAL

@joyeecheung
Copy link
Member Author

Ping @bnoordhuis

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

@joyeecheung
Copy link
Member Author

Landed in f59ec2a

@joyeecheung joyeecheung closed this Apr 4, 2019
joyeecheung added a commit that referenced this pull request Apr 4, 2019
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>
BethGriggs pushed a commit that referenced this pull request Apr 4, 2019
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>
addaleax added a commit to addaleax/node that referenced this pull request Jun 9, 2020
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
addaleax added a commit that referenced this pull request Jun 11, 2020
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>
codebytere pushed a commit that referenced this pull request Jun 18, 2020
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>
codebytere pushed a commit that referenced this pull request Jun 30, 2020
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>
codebytere pushed a commit that referenced this pull request Jul 10, 2020
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>
codebytere pushed a commit that referenced this pull request Jul 14, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants