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

Reduce recorder memory usage #1479

Merged
merged 3 commits into from
Feb 4, 2024
Merged

Reduce recorder memory usage #1479

merged 3 commits into from
Feb 4, 2024

Conversation

xjunior
Copy link
Contributor

@xjunior xjunior commented Aug 31, 2022

Before this change in my test suite:

allocated memory by gem
-----------------------------------
 450.63 MB  rspec-mocks-3.11.0
 250.91 MB  activesupport-5.2.8.1
 247.35 MB  activerecord-5.2.8.1

After this change in my test suite:

allocated memory by gem
-----------------------------------
 250.92 MB  activesupport-5.2.8.1
 247.36 MB  activerecord-5.2.8.1
 198.83 MB  rspec-mocks-3.11.0

@pirj pirj requested a review from JonRowe August 31, 2022 18:33
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Nice improvement, thank you

@xjunior
Copy link
Contributor Author

xjunior commented Jan 7, 2023

Hello, @pirj. Any chance we're shipping this soon?

@JonRowe
Copy link
Member

JonRowe commented Jan 7, 2023

Can you construct a benchmark for this? It looks harmles but I'm also confused why it saves memory, its essentially the same code except for using blocks rather than lambdas

@xjunior
Copy link
Contributor Author

xjunior commented Jan 12, 2023

@JonRowe hello! I got to this part of the code by debugging and profiling my own test suite. The description includes a benchmark of it.

@JonRowe
Copy link
Member

JonRowe commented Jan 12, 2023

@xjunior if you check the benchmarks folder you'll see the sort of thing we like to add with these changes to document them for prosperity (and allow us to re-check if it changes at a later date) this helps us to avoid churn because something works "better on my machine"

@xjunior
Copy link
Contributor Author

xjunior commented Oct 27, 2023

@JonRowe I added some. It doesn't show the same results I'm showing above. I believe this is because mine is running on a bigger sample, with rails, rspec-rails and others. The results I pushed show a 30% difference on memory allocation on this specific example.

Before this change in my test suite:

```sh
allocated memory by gem
-----------------------------------
 450.63 MB  rspec-mocks-3.11.0
 250.91 MB  activesupport-5.2.8.1
 247.35 MB  activerecord-5.2.8.1
```

After this change in my test suite:

```sh
allocated memory by gem
-----------------------------------
 250.92 MB  activesupport-5.2.8.1
 247.36 MB  activerecord-5.2.8.1
 198.83 MB  rspec-mocks-3.11.0
```
@pirj
Copy link
Member

pirj commented Jan 25, 2024

@JonRowe WDYT? I'm very much in favour of merging this.

@JonRowe JonRowe merged commit a9e212b into rspec:main Feb 4, 2024
28 of 30 checks passed
JonRowe added a commit that referenced this pull request Feb 4, 2024
JonRowe added a commit that referenced this pull request Feb 4, 2024
Reduce recorder memory usage
JonRowe added a commit that referenced this pull request Feb 4, 2024
@JonRowe
Copy link
Member

JonRowe commented Feb 4, 2024

This has been released in 3.12.7

@xjunior xjunior deleted the patch-1 branch February 4, 2024 21:02
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

4 participants