Call free on GC for object types#958
Conversation
d3adb47 to
27713af
Compare
|
This adds two mechanisms - one marks types as The second mechanism is |
|
BTW, this is leading up to #955 - by itself, in terms of memory leaks this PR only has a benefit if the user calls |
| to = {{ cppClassName }}::New({{= parsedName =}}); | ||
| {% else %} | ||
| to = {{ cppClassName }}::New({{= parsedName =}}, false); | ||
| to = {{ cppClassName }}::New({{= parsedName =}}, {% if selfFreeing %}true{% else %}false{% endif %}); |
There was a problem hiding this comment.
This is pretty verbose. I think there should be another Combyne filter called toBool that just basically returns !!value. I'll push that to your branch.
|
I was thinking about extending the instance counting to all wrappers (not just for types marked as @johnhaley81 what do you think? |
|
@implausible for For example, we should definitely not duplicate on |
|
@srajko wrt Regardless of that though, it does highlight a need for a C++ testing suite so we should start looking into that. Once we get something going in that case I think we should remove |
|
So extending the instance count to all wrappers (this comment notwithstanding) is a good idea. In tests we should at least start documenting memory usage and anything that can help diagnose leaks is 👍 |
|
@srajko so However, this is (what I believe) |
I think that would work well, and would address #958 (comment)
It's related but slightly different. I think, in the endgame, we need every object to be either self-freeing (that is, the wrapper owns it, and nothing else is going to free it while the wrapper is alive), or it needs to have a handle to its owner's wrapper (so that the owner can't be GCd/destroyed as long as the object in question is reachable). Currently though, we have three classes of objects - leaked, For things like |
|
@johnhaley81 I agree, C++ testing would be great. I think I got something going on a branch with https://www.npmjs.com/package/cppunitlite at one point. I would prefer http://banditcpp.org/ though, for its syntactic similarity to the JS tests (maybe cppunitlite and bandit could be used together?) I will extend the count system to all wrappers then, but I do agree with you - once we get the leaks resolved all across nodegit and/or get some C++ tests going we could pull out the exposed statistics. |
|
I'm looking into why coverage tests are dying. @tbranyen have any idea why this is happening? |
|
Aaaaand it fixed itself /shrug |
|
Just a side note that what just happened is scary as hell :) |
|
I fixed up the diagnostic counts. I didn't want to go overboard with what we count, so I'm just counting too values - for self-freeing objects, it counts the number of outstanding instances (constructed-destructed). For non-self-freeing, it only counts the number of co 10BC0 nstructed instances (as this is a possible source of memory leaks). |
|
@implausible do you want to take a look at #958 (comment) / #958 (comment) or do you want me to? |
|
@srajko i'm currently working on increasing lock coverage in GK and as that will also be a big change... I don't think I can contribute today. |
| to = {{ cppClassName }}::New({{= parsedName =}}); | ||
| {% else %} | ||
| to = {{ cppClassName }}::New({{= parsedName =}}, {{ selfFreeing|toBool }}); | ||
| to = {{ cppClassName }}::New({{= parsedName =}}, {{ selfFreeing|toBool }} {% if shouldDuplicate %}, true{% endif %}); |
There was a problem hiding this comment.
{{ shouldDuplicate|toBool }}?
There was a problem hiding this comment.
Tried it, but there is a case where we call that on something that I think is a struct. And I didn't want to add that parameter to struct constructors without adding support for it as well.
There was a problem hiding this comment.
(so adding the 3rd parameter only whenshouldDuplicate is turned on avoids that problem)
Affects commit objects returned by: git_commit_lookup git_commit_lookup_prefix git_commit_nth_gen_ancestor git_commit_parent
Affects tree objects returned by: git_tree_lookup git_tree_lookup_prefix
Affects tag objects returned by: git_tag_lookup git_tag_lookup_prefix
Affects blob objects returned by: git_blob_lookup git_blob_lookup_prefix
It's possible that we will eventually get rid of the selfFreeing parameter, and just go by the type descriptor, but for now we are more comfortable with a smaller change.
the structs git_tree_entry, git_oid, and git_signature all represent structs which are owned by some other object. In order to simplify our memory model, we should always duplicate these 3 structs when we pull them out of libgit2. This detaches these types of structs from their owning object, such that when we delete a commit, none of the oids or signatures that we retrieved from that commit would cause bad memory access.
|
I've added the |
|
This LGTM now. Thanks @srajko |
This reverts commit 487aa31.
No description provided.