-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Free pointers in parser activations #4486
Free pointers in parser activations #4486
Conversation
Oh awesome! Nice work. Would you consider writing up a tutorial on how to use Valgrind on a Cython project? As I mentioned in Slack, I've always wanted to use it but have had trouble getting started. I feel sheepish about this memory leak, as it could have been discovered the old-fashioned way by just grepping for malloc and calloc. I normally try to be careful when I'm using them, because they break the normal pattern of |
I only kinda sorta know what I'm doing, but I can sketch out the basics of how I did this, sure! |
Ah, I think I see how I missed this. The allocations and the frees are happening in different modules, which makes the mistake much more likely. I suggest we make it a bit easier to see that the cdef ActivationsC alloc_activations(int n_states) nogil:
cdef ActivationsC acts
memset(&acts, 0, sizeof(acts))
resize_activations(&acts, n_states)
return acts
cdef void free_activations(const ActivationsC* acts) nogil:
free(acts.token_ids)
free(acts.scores)
free(acts.unmaxed)
free(acts.hiddens)
free(acts.is_valid) We would then remove the |
However you think is best... |
* Rewrite/restructure to have allocation and free in parallel functions in `_parser_model` rather than partially in `_parseC()` in `Parser`. * Remove `resize_activations` from `_parser_model.pxd`.
* Free pointers in ActivationsC * Restructure alloc/free for parser activations * Rewrite/restructure to have allocation and free in parallel functions in `_parser_model` rather than partially in `_parseC()` in `Parser`. * Remove `resize_activations` from `_parser_model.pxd`.
* Free pointers in parser activations (#4486) * Free pointers in ActivationsC * Restructure alloc/free for parser activations * Rewrite/restructure to have allocation and free in parallel functions in `_parser_model` rather than partially in `_parseC()` in `Parser`. * Remove `resize_activations` from `_parser_model.pxd`. * Try and fix pytest Co-authored-by: Ines Montani <[email protected]>
Description
Free the remaining pointers in the
ActivationsC
activations at the end of_parseC()
inParser
.This is the only definite spacy-related memory leak I found with valgrind using examples from #3618. (I think there may be some remaining issues related to cython/numpy.)
Types of change
Bugfix.
Checklist