Fix memory leak in parameter system #1090
Conversation
|
By analyzing the blame information on this pull request, we identified @tbekolay to be a potential reviewer |
|
A callback! Well played, Python Also I think makes sense to squash to a single commit during the merge. |
|
This code looks fine, but I don't quite understand why this whole thing is necessary. Why were things not getting garbage collected before, and how does this help? |
|
Oh, I see, the key was getting freed, but not the value? So we were just storing a bunch of extra values. Maybe put a comment to that effect somewhere, either when you make the callback or at the beginning of the callback. |
Yes.
Isn't that the reviewer's task now given the PR review guide lines? |
Well, technically it's whoever has the time. I had to run earlier so I put it back to you. But I can do it tomorrow. |
|
@jgosmann: Does this look good? |
|
LGTM 🍰 |
WeakKeyIDDictionary was not removing values when the key was freed. We create a weak reference to the key with a callback here that will free the value when no strong references to the key are left. To be able to do this we need the id of the key which will be retrieved with the _ref2id dictionary. Note that weak reference use the hash function of the object they are referencing. Thus, we have to use the id of the weak reference to index that dictionary. Finally, to support the del operation we need to be able to map the object id to the weak reference which requires another dictionary _id2ref (instantiating another weak ref would give us an object with a different id). This dictionary has another important function: It stores strong references the weak reference to ensure that the callback gets called (it would not if the weak reference gets garbage collected beforehand and we need to store ids as keys in the _ref2id dictionary). Fixes #1089. Also added a test for WeakKeyIDDictionary value garbage collection.
|
I'm happy to merge @hunse or you can go ahead; I'd squash it all down to one commit, I think. |
|
I'll merge. Just one comment about commit messages: fd0bb32 has a very long commit message that goes into detail about why two dictionaries are used. This kind of documentation is useful, but I'm not sure if a commit message is the best place for it. If I'm working on that section of code, it's not likely that I'll see that message. I could do a git blame, but often one would need a second- or third-generation blame to get back to a message like that. For example, I added a commit here that fixes the function order, but that's going to make the blame for those important changes one generation older. |
|
Also, I did keep the full commit message in the merge, so that comment was more of a general comment than something that we actually need to address here. |
|
I'd much rather have the documentation in the commit message than in the PR text... there's no downside to long commit messages because most tools will only show the summary line anyway. It's probably true that placing that documentation elsewhere would be good too, but I definitely don't want to discourage anyone from writing long commit messages, as that's definitely something that I read when looking at PRs commit-by-commit. |
|
But yeah, putting that information in the code itself is often preferable, I will agree on that :) 🌈 🐙 |
Description:
Fixes #1089.
We create a weak reference to the key with a callback here that will free the value when no strong references to the key are left. To be able to do this we need the id of the key which will be retrieved
with the _ref2id dictionary. Note that weak reference use the hash function of the object they are referencing. Thus, we have to use the id of the weak reference to index that dictionary. Finally, to support
the del operation we need to be able to map the object id to the weak reference which requires another dictionary _id2ref (instantiating another weak ref would give us an object with a different id). This dictionary has another important function: It stores strong references the weak reference to ensure that the callback gets called (it would not if the weak reference gets garbage collected beforehand and we
need to store ids as keys in the _ref2id dictionary).
Motivation and context:
See #1089.
How has this been tested?
Added a unit test checking that values in the
WeakKeyIDDictionaryare cleaned up.Where should a reviewer start?
Look at #1089 and the test to understand the problem, then look at the changes to
WeakKeyIDDictionaryand information above to see how this is fixed.How long should this take to review?
Types of changes:
Checklist: