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

Improve memory usage in Bundler::Settings, and thus improve boot time #6884

Merged
merged 14 commits into from Aug 20, 2023

Conversation

technicalpickles
Copy link
Contributor

@technicalpickles technicalpickles commented Aug 16, 2023

What was the end-user or developer problem that led to this PR?

I was running memory_profiler on our Rails app boot-time, and saw some opportunities.

What is your fix for the problem, implemented in this PR?

I used this code to scope memory allocations in our rails app to just bundler:

require 'memory_profiler'

MemoryProfiler.report(allow_files: 'bundler/settings.rb') do
  require_relative 'config/environment'
end.pretty_print

And then compared it with these changes by adding adding calling it with ruby -Ipath/to/checkout/rubygems/bundler/lib.

Memory

Before:

Total allocated: 4883872 bytes (65211 objects)
Total retained:  22644 bytes (226 objects)


allocated memory by file
-----------------------------------
   4883872  /Users/josh.nichols/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/bundler-2.4.18/lib/bundler/settings.rb

allocated objects by file
-----------------------------------
     28606  /Users/josh.nichols/workspace/rubygems/bundler/lib/bundler/settings.rb

retained memory by file
-----------------------------------
     21333  /Users/josh.nichols/workspace/rubygems/bundler/lib/bundler/settings.rb

retained objects by file
-----------------------------------
       226  /Users/josh.nichols/workspace/rubygems/bundler/lib/bundler/settings.rb

After:

Total allocated: 2326771 bytes (28606 objects)
Total retained:  20207 bytes (221 objects)

allocated memory by file
-----------------------------------
   2326771  /Users/josh.nichols/workspace/rubygems/bundler/lib/bundler/settings.rb

allocated objects by file
-----------------------------------
     28606  /Users/josh.nichols/workspace/rubygems/bundler/lib/bundler/settings.rb


retained memory by file
-----------------------------------
     20207  /Users/josh.nichols/workspace/rubygems/bundler/lib/bundler/settings.rb

retained objects by file
-----------------------------------
       221  /Users/josh.nichols/workspace/rubygems/bundler/lib/bundler/settings.rb

Runtime

For runtime, I with the same file above, but without the memory_profiling:

require_relative 'config/environment'

I used hyperfine to compare before/after: hyperfine "ruby boot-time-benchmark.rb" "ruby -I/Users/josh.nichols/workspace/rubygems/bundler/lib boot-time-benchmark.rb":

CleanShot 2023-08-16 at 15 25 27

Summary

This reduces the amount of memory allocated during Rails startup in our app by 52%, from 4_883_872 bytes to 2_326_771 bytes.

It also reduces startup time by 6%, from 8.807s to 8.256s.

Make sure the following tasks are checked

@indirect
Copy link
Member

Nice! A boon for the massively-environmented users everywhere 😂

@technicalpickles
Copy link
Contributor Author

I think I got over-ambitious with this PR, and now I'm getting tons of failures that aren't super obvious where to begin (ie expected some big string, but got empty string) so will break some of these out.

@technicalpickles
Copy link
Contributor Author

@martinemde pushed up those changes!

@indirect indirect merged commit 06231fd into rubygems:master Aug 20, 2023
92 checks passed
@technicalpickles technicalpickles deleted the settings-memory branch August 21, 2023 16:30
@deivid-rodriguez
Copy link
Member

Hei, I didn't fin time to chime in here, but THIS IS SO AWESOME! Thank you so much!

technicalpickles added a commit to technicalpickles/rubygems that referenced this pull request Aug 29, 2023
I previously identified and improved this method over in rubygems#6884
but while reviewing another memory_profiler profile, I realized another
gain we can eek out.

This method keeps comes up in part because `configs` is allocating a new
Hash every time. My last change took advantage of that by using `map!`
on it. `configs` is called quite often, including in this `[]` method,
so there's a benefit to memoizing it.

Back in `[]`, logically we are trying to find the first Hash in `configs`
that has a value for the given key. Currently, we end up `map` and
`compact` to just get that value.

Instead, we can use a loop over `configs`, and break when we find the
value for the key.
matzbot pushed a commit to ruby/ruby that referenced this pull request Aug 30, 2023
…e and memory usage

I previously identified and improved this method over in rubygems/rubygems#6884
but while reviewing another memory_profiler profile, I realized another
gain we can eek out.

This method keeps comes up in part because `configs` is allocating a new
Hash every time. My last change took advantage of that by using `map!`
on it. `configs` is called quite often, including in this `[]` method,
so there's a benefit to memoizing it.

Back in `[]`, logically we are trying to find the first Hash in `configs`
that has a value for the given key. Currently, we end up `map` and
`compact` to just get that value.

Instead, we can use a loop over `configs`, and break when we find the
value for the key.

rubygems/rubygems@b913cfc87b
deivid-rodriguez pushed a commit that referenced this pull request Sep 21, 2023
Improve memory usage in Bundler::Settings, and thus improve boot time

(cherry picked from commit 06231fd)
deivid-rodriguez pushed a commit that referenced this pull request Sep 21, 2023
Improve memory usage in Bundler::Settings, and thus improve boot time

(cherry picked from commit 06231fd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants