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 leak in mock #68

Merged
merged 3 commits into from
Feb 29, 2024
Merged

Fix memory leak in mock #68

merged 3 commits into from
Feb 29, 2024

Conversation

MaxLap
Copy link
Contributor

@MaxLap MaxLap commented Feb 25, 2024

As discussed in #67.

The commits are incremental and coherent changes, so going commit by commit may be the easiest way to review.

rspec on this repo, and the mspec on ruby-spec are both passing with these changes.

To see the impact easily, edit ruby-spec/spec/core/hash/compare_by_identity_spec.rb to only leave the spec uses #equal? semantics, but doesn't actually call #equal? to determine identity. (line 50)

Then run ../mspec/bin/mspec --repeat 1000000 core/hash/compare_by_identity_spec.rb.

It should be easy to see the memory that just keeps going up quickly (before this change) using top or similar monitoring tool.

This avoids needing has_key? which iterates on hash keys
This also clarifies code a lot, since replaced and hash_key? had a "sym" argument which wasn't a regular method name, unlike every other instance of "sym" in the file. That was confusing.
…ow expected a key

This decouples the key, which must be unique, from the replacement method name, which we would prefer not to be unique (See ruby#67)
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Looks great, thank you.

The second commit description was quite to understand, basically it already talks about the 3rd commit's diff. Probably best to squash those two together.

@eregon eregon merged commit 58050d7 into ruby:master Feb 29, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants