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

Properly handle retainers for WeakMaps/WeakSets #2

Open
nolanlawson opened this issue Oct 6, 2020 · 1 comment
Open

Properly handle retainers for WeakMaps/WeakSets #2

nolanlawson opened this issue Oct 6, 2020 · 1 comment

Comments

@nolanlawson
Copy link

First off, thank you for creating this tool! It works amazingly well, and solves many of the problems with working with heap snapshots that I mentioned in my blog post and this issue I opened on Chromium.

One issue I notice, though, is that when identifying the retainers for an object with a given ID, it doesn't properly take WeakMaps/WeakSets into account. For instance:

  1. When an object is the key in a WeakMap, then the WeakMap itself (and its retainers) are not really retaining the object. So in this case the WeakMap is irrelevant and should be removed. However, the tool does not do this.
  2. When the object is the value in a WeakMap, then the key is what's retaining the object (plus the retainers of the WeakMap itself). However, the tool does not seem to follow the retainers for the key (meaning you would have to do a separate analysis for that object ID, which can get impractical in an object graph with many WeakMaps/WeakSets).

Thanks again for the tool! I would be excited to see it integrated into the Chrome DevTools themselves, and to improve the handling of WeakMaps/WeakSets. 🙂

@ykahlon
Copy link
Owner

ykahlon commented Oct 6, 2020

@nolanlawson this is a good find. Thank you!!
I will try to see what the dep graph provides in order for the tool to treat the relation of
WeakMap -> key as a weak reference. The tool calls into:

this.disconnectEdgesWithType('weak');

which supposes to treat this correctly but I guess that for WeakMap/WeakSet it is handled differently.
Feel free to try and contribute, I will try to look into this in the next couple of days though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants