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 string allocations and retentions #580

Merged
merged 1 commit into from Apr 19, 2019
Merged

Conversation

casperisfine
Copy link
Contributor

While running memory_profiler agaisnt our app, I noticed quite a lot of duplicated strings coming from whois:

Retained String Report
-----------------------------------

1059  "whois.arin.net"
1059  /tmp/bundle/ruby/2.5.0/bundler/gems/json-b5f423ccf08f/lib/json/common.rb:155


689  "whois.lacnic.net"
689  /tmp/bundle/ruby/2.5.0/bundler/gems/json-b5f423ccf08f/lib/json/common.rb:155


387  "whois.donuts.co"
196  /tmp/bundle/ruby/2.5.0/gems/psych-3.1.0/lib/psych.rb:456
191  /tmp/bundle/ruby/2.5.0/bundler/gems/json-b5f423ccf08f/lib/json/common.rb:155


314  "whois.ripe.net"
314  /tmp/bundle/ruby/2.5.0/bundler/gems/json-b5f423ccf08f/lib/json/common.rb:155

So here's I'm trying to reduce these duplications as to reclaim a bit of memory. The whole thing rely on String#-@ which starting in MRI 2.5 will freeze and possibly intern the string.

On 2.4 and older (or some oldish alternative implementation) it will fallback to simply freezing the string, so no memory will be saved, but for the caller the string will still be frozen so that should prevent any kind of weirdness when upgrade the ruby version.

@weppos weppos self-assigned this Apr 10, 2019
@weppos
Copy link
Owner

weppos commented Apr 10, 2019

Thanks @casperisfine , this looks great. Are you able to share some insights about the allocation improvement?

@casperisfine
Copy link
Contributor Author

@weppos I'd need to re-run the profile with this gem patched. It would take ~20m. However I'm confident all these duplications will disappear as I already did some very similar PR on several gems.

But tell me and I can trigger a profile.

@casperisfine
Copy link
Contributor Author

Ok, so I just ran the profile anyway.

These strings are effectively gone from the retained string reports as expected.

As for the overall impact, the total retained memory difference is that this branch retain 490kiB more memory while we expected the inverse.

That being said the baseline was using whois-3.6.5, so that's likely why. I'll trigger another profile against 7f67e4e

@casperisfine
Copy link
Contributor Author

Ok, so compared to 7f67e4e:

Retained memory: -350kB
Retained objects -2378

So I'd say it's a decent saving.

@casperisfine
Copy link
Contributor Author

Are you able to share some insights about the allocation improvement?

Oh, I just figured you were asking about allocations vs retention. I totally forgot to explain to allocation part:

On master:

Allocated String Report
-----------------------------------
8628  "host"
4159  /tmp/bundle/ruby/2.5.0/bundler/gems/whois-7f67e4e6e8ea/lib/whois/server.rb:78
3845  /tmp/bundle/ruby/2.5.0/bundler/gems/json-b5f423ccf08f/lib/json/common.rb:155

That is why I added the magic comment on the top of the file, this way it saves 4k allocations of "host".

Looking at the overall allocation savings I see a 830MB and 93084 objects saving, but that seam way too good to be true, memory_profiler must be confused by something.

@casperisfine
Copy link
Contributor Author

but that seam way too good to be true

Ok, yeah the difference come from on having some cache loaded, while the other was fresh, so simply ignore that.

The actual allocation reduction should be is

before: 1.63 MB whois-7f67e4e6e8ea

after: 1.09 MB whois-879f6e7c1ef3

So a bit over 500KB saved.

@weppos
Copy link
Owner

weppos commented Apr 19, 2019

Sweet, thanks! ❤️

@weppos weppos merged commit a6863d2 into weppos:master Apr 19, 2019
@weppos
Copy link
Owner

weppos commented Apr 19, 2019

Shipped 4.1.0

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

3 participants