search

mime-types / ruby-mime-types

Ruby MIME type registry library
Other
323 stars 122 forks source link

Intern content type strings #141

Closed casperisfine closed 5 years ago

casperisfine commented 5 years ago

While running a memory profiler against our app, I noticed mime-types was holding onto a lot of duplicated strings:

3163  "application"
1577  /tmp/bundle/ruby/2.5.0/gems/mime-types-3.2.2/lib/mime/type.rb:543
1577  /tmp/bundle/ruby/2.5.0/gems/mime-types-3.2.2/lib/mime/type.rb:546

The idea here is to apply String#@- to as much as possible deduplicate these strings.

Note that regardless of wether they are duplicates or not, they will be frozen (which IMO is a good thing).

halostatue commented 5 years ago

Which memory profiler were you running? I’ve got a branch for a value pool implementation that would benefit from this, and I’d be able to compare interning vs value pool for both performance and memory.

I can’t accept this PR as is, because this change would require a hard bump of version to 4.0 since I still officially support Ruby 2.1 and higher.

This could be handled by adding a class function MIME::Types.intern(string) that is implemented differently for Ruby versions that support String#@- and those that don’t.

casperisfine commented 5 years ago

This could be handled by adding a class function MIME::Types.intern(string) that is implemented differently for Ruby versions that support String#@- and those that don’t.

Indeed, I totally missed the 2.1 support.

I have a string interner implementation I used on another project, I can update this PR to fallback to it on pre 2.5.

casperisfine commented 5 years ago

Which memory profiler were you running?

https://github.com/SamSaffron/memory_profiler it has a few bugs, but it's decent enough.

casperisfine commented 5 years ago

I can update this PR to fallback to it on pre 2.5.

Or alternatively, I can simply not intern anything on pre 2.5. It's already how 2.4 and 2.5 passe CI. String#-@ freeze the strings, but don't really intern anything.

Let me know which you prefer, but IMHO I don't think it's worth lots of effort to save memory on very old versions.

casperisfine commented 5 years ago

I pushed a new commit as to not break if interning is not available.

halostatue commented 5 years ago

I will release this update in the near future. Thanks for the contribution!

casperisfine commented 5 years ago

👋 sorry to bother, but is there anything blocking a release I could help with?

halostatue commented 5 years ago

Unfortunately, it’s mostly my time. I’ll try to get something soon. Sorry for the delay.

casperisfine commented 5 years ago

No worries, I know how it is.

halostatue commented 5 years ago

Released.

casperisfine commented 5 years ago

❤️ Thanks a lot!