Cache interpolator methods and reduce memory allocations #1888

dgynn · 2015-06-07T23:46:37Z

The Paperclip::Interpolations module generates a lot of objects. As noted in #1882 there is memory allocated to get the list of interpolation methods and then again when gsub is called.

This commit memoizes the array of interpolation methods and the key-to-symbol mappings. It then uses gsub! to manipulate the result only when a key matches. The interpolation method cache is reset if any new interpolation is added.

I've benchmarked it using a simple Attachment object configured to use :s3 and calling my_attachment.url(:thumb). The cached version is 4.5x faster and uses about 85% less memory. This is separate from the improvements in #1873 (which were also great).

There are still some tunings that could be done on the individual interpolations, like freezing strings. This commit doesn't address those.

Benchmark IPS comparison

Benchmarking interpolaters
Calculating -------------------------------------
original interpolations
                       319.000  i/100ms
cached interpolations
                         1.539k i/100ms
-------------------------------------------------
original interpolations
                          3.143k (± 5.3%) i/s -     15.950k
cached interpolations
                         15.429k (± 5.5%) i/s -     76.950k

Comparison:
cached interpolations:    15429.4 i/s
original interpolations:     3143.1 i/s - 4.91x slower

MemoryProfiler before:

Total allocated: 71828 bytes (520 objects)
Total retained:  756 bytes (6 objects)

allocated memory by gem
-----------------------------------
     71788  paperclip/lib
        40  ruby-2.2.2/lib

Memory Profiler after

Total allocated: 9937 bytes (98 objects)
Total retained:  998 bytes (8 objects)

allocated memory by gem
-----------------------------------
      9728  paperclip/lib
       209  ruby-2.2.2/lib

houndci-bot · 2015-06-07T23:46:40Z

lib/paperclip/interpolations.rb

+    end
+
+    def self.tag_pattern_cache
+      @tag_pattern_cache ||= Hash.new {|hash, key| hash[key]= ":#{key}"}


Surrounding space missing for operator '='.
Space between { and | missing.
Space missing inside }.

bitcoder · 2015-06-22T14:37:19Z

in my use case, as referred in #1882, it behaves quite better (with some room for improvements :) ). see bellow for the difference between current master and this PR.

with current master version, commit 7f732ee

allocated memory by gem
-----------------------------------
  31226200  activerecord-4.2.0
  17798700  paperclip-7f732ee5a904

allocated memory by file
-----------------------------------
  16797060  /Users/sergio/work/capthost/vendor/ruby/2.1.0/bundler/gems/paperclip-7f732ee5a904/lib/paperclip/interpolations.rb
  14003570  /Users/sergio/work/capthost/vendor/ruby/2.1.0/gems/activerecord-4.2.0/lib/active_record/connection_adapters/postgresql/database_statements.rb

allocated memory by location
-----------------------------------
  13856475  /Users/sergio/work/capthost/vendor/ruby/2.1.0/gems/activerecord-4.2.0/lib/active_record/connection_adapters/postgresql/database_statements.rb:168
  13800780  /Users/sergio/work/capthost/vendor/ruby/2.1.0/bundler/gems/paperclip-7f732ee5a904/lib/paperclip/interpolations.rb:33
   6105520  /Users/sergio/work/capthost/vendor/ruby/2.1.0/gems/activerecord-4.2.0/lib/active_record/result.rb:116
   2867520  /Users/sergio/work/capthost/vendor/ruby/2.1.0/bundler/gems/paperclip-7f732ee5a904/lib/paperclip/interpolations.rb:23
allocated objects by location
-----------------------------------
     90066  /Users/sergio/work/capthost/vendor/ruby/2.1.0/bundler/gems/paperclip-7f732ee5a904/lib/paperclip/interpolations.rb:33
     85855  /Users/sergio/work/capthost/vendor/ruby/2.1.0/gems/activerecord-4.2.0/lib/active_record/connection_adapters/postgresql/database_statements.rb:168
     66555  /Users/sergio/work/capthost/vendor/ruby/2.1.0/bundler/gems/paperclip-7f732ee5a904/lib/paperclip/interpolations.rb:23

with this PR, commit 9f96d82

allocated memory by gem
-----------------------------------
  31226200  activerecord-4.2.0
  10846925  paperclip-9f96d8290537

allocated memory by file
-----------------------------------
  14003570  /Users/sergio/work/capthost/vendor/ruby/2.1.0/gems/activerecord-4.2.0/lib/active_record/connection_adapters/postgresql/database_statements.rb
   9845285  /Users/sergio/work/capthost/vendor/ruby/2.1.0/bundler/gems/paperclip-9f96d8290537/lib/paperclip/interpolations.rb

allocated memory by location
-----------------------------------
  13856475  /Users/sergio/work/capthost/vendor/ruby/2.1.0/gems/activerecord-4.2.0/lib/active_record/connection_adapters/postgresql/database_statements.rb:168
   9793085  /Users/sergio/work/capthost/vendor/ruby/2.1.0/bundler/gems/paperclip-9f96d8290537/lib/paperclip/interpolations.rb:35

edsimpson · 2015-07-11T06:59:47Z

+1
I would love to have this released.

houndci-bot · 2015-07-18T09:55:21Z

spec/paperclip/plural_cache_spec.rb

  end

-  it 'pluralizes and underscore words' do
+  it 'pluralizes and underscore class names' do


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

dgynn · 2015-07-18T10:52:09Z

I've added some additional performance tunings related to interpolation. These changes bring another 50% speed improvement by optimizing the individual interpolation methods. These gains are primarily through using sub rather than gsub (when appropriate), freezing strings, and more efficiently determining the extension. The PluralCache has also been modified to be used more effectively than just caching the pluralize call.

The benchmark script I'm using is here... https://gist.github.com/dgynn/1d8380ee57999597db2f
The benchmark still use :s3 storage and now includes :style attribute configuration options.

Running that benchmark against master (947fbf6), the interim commit (June 7), and the latest commit using MemoryProfiler results in the following counts for generating one .url(:thumb) call.

Master
Total allocated: 74513 bytes (547 objects)
Total retained:  2069 bytes (17 objects)

Interim
Total allocated: 12640 bytes (125 objects)
Total retained:  2449 bytes (22 objects)

Latest
Total allocated: 8128 bytes (84 objects)
Total retained:  2129 bytes (20 objects)

The Benchmark IPS results show a 6x+ improvement of...

Master
      original style      3.657k (± 1.3%) i/s -     18.480k
style without format      3.536k (± 1.6%) i/s -     17.813k
   style with format      3.198k (±12.6%) i/s -     15.792k

Interim
      original style     15.788k (± 1.9%) i/s -     78.999k
style without format     14.885k (± 3.0%) i/s -     75.550k
   style with format     15.459k (± 1.7%) i/s -     78.008k

Latest
      original style     23.203k (± 2.4%) i/s -    118.206k
style without format     22.330k (± 1.1%) i/s -    112.372k
   style with format     23.181k (± 1.4%) i/s -    117.208k

jyurek · 2015-08-21T14:58:18Z

Hey @dgynn, thanks for the work here. But as I just pulled in #1903 this doesn't rebase cleanly. Can you bring it up to date so I can pull it in? Thanks.

houndci-bot · 2015-08-21T17:11:06Z

lib/paperclip/storage/s3.rb

@@ -148,7 +148,7 @@ def sanitize_hash(hash)
            Proc.new do |style, attachment|
              permission  = (@s3_permissions[style.to_s.to_sym] || @s3_permissions[:default])
              permission  = permission.call(attachment, style) if permission.respond_to?(:call)
-              (permission == DEFAULT_PERMISSION) ? 'http' : 'https'
+              (permission == DEFAULT_PERMISSION) ? 'http'.freeze : 'https'.freeze


Line is too long. [81/80]
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

dgynn · 2015-08-21T17:16:19Z

Thanks @jyurek. This branch is mergeable now and the specs should be passing. Let me know if you want this squashed/rebased at all.

jyurek · 2015-09-01T18:35:02Z

Hey @dgynn I'm still getting some conflicts in lib/paperclip/storage/s3.rb when I rebase this on master. Can you take another look? Thanks.

by using the attachment name (symbol) and Class as keys we reduce the number of Strings created before hitting the cache

this commit primarily uses frozen strings to reduce object creation during interpolation. the :basename method now uses File.basename(file, ".*") rather than a Regexp. basename may be called multiple times.

the name string is used multiple times in interpolation so storing it reduces object creation

dgynn · 2015-09-01T19:52:16Z

@jyurek I've rebased against master and cleaned up that conflict. Thanks for merging this.

dgynn · 2015-09-09T02:44:41Z

@tute Can you see about merging this branch too? The Travis failure was just 1 test not running fast enough. This should be good to merge. Let me know if it isn't.

tute · 2015-09-09T02:49:15Z

Thanks! It does rebase cleanly now, and I restarted the two jobs that failed.

Would love a review of @schneems too, in case he spots any potential issue or potential further performance gains.

Thank you all!

dgynn · 2015-11-12T16:14:52Z

@tute I'd love to get this merged as part of the 4.2.x release before the upgrade to 5.0 if that is possible.

schneems · 2015-11-14T16:48:27Z

👍 all the perf pieces look good. Any chance someone is relying on internal class methods like PluralCache#pluralize ? if not this looks good and tests pass, if they might then we should deprecate instead of removing outright.

Great job 👏

tute · 2015-11-15T03:20:34Z

Thanks for your work @dgynn, and for your review @schneems!

Any chance someone is relying on internal class methods like PluralCache#pluralize?

That's a good point. Any thoughts on that, @jyurek?

I'd love to get this merged as part of the 4.2.x release before the upgrade to 5.0

Sounds good to me. In that case, the PR should be rebased on top of (and reopened against) v4.3 branch. Can you please do that? I'm sorry it this means undoing merge conflicts resolution done for master. But that way we can do a good point release as soon as we merge!

dgynn · 2015-11-16T08:18:52Z

Thanks for the review @schneems.

For the PluralCache, I was thinking it was unlikely that anyone would be relying on it. @jyurek, let me know if you think it should be added back with a deprecation warning.

I've just submitted PR #2051 to get master back to green. If that PR is OK and can get merged I'll rebase on that and then look at doing the same on v4.3.

schneems · 2015-11-16T17:02:37Z

It doesn't look like many people would use it, just wanted an opinion from someone closer to the project.

tute · 2015-11-17T04:58:01Z

I agree that this API change should be fine.

@dgynn PR is green! Rebase on top of 4.3 and release?

dgynn · 2015-11-17T17:01:30Z

@tute I've opened 2 PRs that are rebased against master #2054 and against v4.3 #2056. I'm good with either or both of them getting merged and a release of 4.3 would be great.

Sorry for so many PRs. I didn't want to rebase this branch because I have a few apps pointing at it that need to get updated.

tute · 2015-11-18T05:31:04Z

@tute I've opened 2 PRs that are rebased against master #2054 and against v4.3 #2056. I'm good with either or both of them getting merged and a release of 4.3 would be great.

Thank you! I merged into v4.3 #2056, and when that branch is green on CI (and when I have the time, in less than a week I'd expect!), I'll release a patch release with this, that we can then merge into master. Thanks so much! 😃 👏 🎉 ❤️

houndci-bot reviewed Jun 7, 2015
View reviewed changes

bitcoder mentioned this pull request Jun 23, 2015

paperclip 4.2.1 memory/object use quite high #1882

Closed

houndci-bot reviewed Jul 18, 2015
View reviewed changes

dgynn mentioned this pull request Jul 18, 2015

Interpolate URLs using a string rather than a regex #1940

Closed

jyurek added the Needs More Work label Aug 21, 2015

houndci-bot reviewed Aug 21, 2015
View reviewed changes

jyurek removed the Needs More Work label Aug 21, 2015

jyurek added the Needs More Work label Sep 1, 2015

dgynn and others added 8 commits September 1, 2015 12:02

improve interpolation speed and memory usage

8a405d3

speed up interpolation by consolidating interpolator caches

6faafc4

optimize Interpolations::PluralCache to require few object creations

ce3681e

by using the attachment name (symbol) and Class as keys we reduce the number of Strings created before hitting the cache

individual interpolation method performance tunings

56ad18a

this commit primarily uses frozen strings to reduce object creation during interpolation. the :basename method now uses File.basename(file, ".*") rather than a Regexp. basename may be called multiple times.

prefer sub to gsub when possible

aaaffb6

save the string value of the attachment name

b524cac

the name string is used multiple times in interpolation so storing it reduces object creation

replace strings with symbols

adba458

Added a test for a bug in interpolations

c77ca63

dgynn force-pushed the interpolations_tuning branch from 372c499 to c77ca63 Compare September 1, 2015 19:05

tute removed the Needs More Work label Nov 15, 2015

dgynn mentioned this pull request Nov 16, 2015

Cache interpolator methods and reduce memory allocations - rebased #2054

Closed

dgynn mentioned this pull request Nov 17, 2015

Cache interpolator methods and reduce memory allocations - rebased on v4.3 #2056

Merged

dgynn closed this Nov 19, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache interpolator methods and reduce memory allocations #1888

Cache interpolator methods and reduce memory allocations #1888

dgynn commented Jun 7, 2015

houndci-bot Jun 7, 2015

bitcoder commented Jun 22, 2015

edsimpson commented Jul 11, 2015

houndci-bot Jul 18, 2015

dgynn commented Jul 18, 2015

jyurek commented Aug 21, 2015

houndci-bot Aug 21, 2015

dgynn commented Aug 21, 2015

jyurek commented Sep 1, 2015

dgynn commented Sep 1, 2015

dgynn commented Sep 9, 2015

tute commented Sep 9, 2015

dgynn commented Nov 12, 2015

schneems commented Nov 14, 2015

tute commented Nov 15, 2015

dgynn commented Nov 16, 2015

schneems commented Nov 16, 2015

tute commented Nov 17, 2015

dgynn commented Nov 17, 2015

tute commented Nov 18, 2015

Cache interpolator methods and reduce memory allocations #1888

Cache interpolator methods and reduce memory allocations #1888

Conversation

dgynn commented Jun 7, 2015

houndci-bot Jun 7, 2015

Choose a reason for hiding this comment

bitcoder commented Jun 22, 2015

edsimpson commented Jul 11, 2015

houndci-bot Jul 18, 2015

Choose a reason for hiding this comment

dgynn commented Jul 18, 2015

jyurek commented Aug 21, 2015

houndci-bot Aug 21, 2015

Choose a reason for hiding this comment

dgynn commented Aug 21, 2015

jyurek commented Sep 1, 2015

dgynn commented Sep 1, 2015

dgynn commented Sep 9, 2015

tute commented Sep 9, 2015

dgynn commented Nov 12, 2015

schneems commented Nov 14, 2015

tute commented Nov 15, 2015

dgynn commented Nov 16, 2015

schneems commented Nov 16, 2015

tute commented Nov 17, 2015

dgynn commented Nov 17, 2015

tute commented Nov 18, 2015