search

ykahlon / heap-cleaner

61 stars 9 forks source link

Properly handle retainers for WeakMaps/WeakSets #2

Open nolanlawson opened 3 years ago

nolanlawson commented 3 years ago

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 commented 3 years ago

@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.