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

Fix memory leaks #17

Merged
merged 4 commits into from Feb 11, 2018
Merged

Fix memory leaks #17

merged 4 commits into from Feb 11, 2018

Conversation

kivikakk
Copy link
Contributor

@kivikakk kivikakk commented Feb 7, 2018

Fixes #16.

/cc @philipturnbull @mislav — I'm not an expert on Ruby extensions. In b456869 I've stored the state in a struct tracked by the GC and just left it to the GC to cleanup. In c8307a6 I use rb_protect instead and clean up. Either way preferred? The latter seems cleaner.

@kivikakk
Copy link
Contributor Author

kivikakk commented Feb 7, 2018

Confirmed by manual inspection this doesn't leak compared to master. (Ran the two test cases in a loop millions of times and watched master consume 100s of MB of RAM, whereas this one stays steady.)

@philipturnbull
Copy link
Contributor

I'm not a Ruby extension expert either but I prefer the second approach fwiw. I used the same approach to fix a leak with VersionSorter.compare "\0" "\0"

That's all the leaks in #16 plugged ✨

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a C expert at all, but I have confidence in your methodology. Merge away! :shipit:

@kivikakk kivikakk merged commit 2088e02 into master Feb 11, 2018
@kivikakk kivikakk deleted the kivikakk/leaks branch February 11, 2018 23:57
kivikakk added a commit that referenced this pull request Feb 12, 2018
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

Successfully merging this pull request may close these issues.

None yet

3 participants