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

Speed regression for #attachment accessor (getter) between 2.2.2 and 2.3.1 version #16

Open
radarek opened this Issue Feb 21, 2015 · 0 comments

Comments

Projects
None yet
1 participant
@radarek

radarek commented Feb 21, 2015

I noticed big speed regression for generated #attachment getter. Here are benchmarks comparing paperclip_database version 2.2.2 and 2.3.1:

2.2.2

> puts Benchmark.measure { 10.times { MyModel.last.csv_report } }
  0.020000   0.000000   0.020000 (  0.027583)

2.3.1

> puts Benchmark.measure { 10.times { MyModel.last.csv_report } }
  0.770000   0.100000   0.870000 (  0.924550)

(I call ActiveRecord::Base.last method in benchmark because only first call to csv_report is slow)

Profiling shows that 98% of time is spent in Paperclip::Storage::Database#setup_attachment_class method.

Here is its implementation:

def setup_attachment_class
  instance.class.ancestors.each do |ancestor|
    # Pick the top-most definition like
    # Paperclip::AttachmentRegistry#definitions_for
    names_for_ancestor = ancestor.attachment_definitions.keys rescue []
    if names_for_ancestor.member?(name)
      @attachment_class = ancestor
    end
  end
end

This method is doing something very very very nasty. It iterates over all ancestors (which in typical rails application returns collection with size above 70 elements) and it call attachment_definitions method. Of course most of the elements don't have this method so for most of them there will be raise error. Raising and rescuing errors in ruby are relatively slow so they shouldn't be used for normal flow.

Here is some more usefull metrics:

ActiveRecord::Base.ancestors.size
=> 70

> puts Benchmark.measure { ActiveRecord::Base.ancestors.each { |ancestor| ancestor.attachment_definitions rescue [] } }
  0.060000   0.010000   0.070000 (  0.080427)

One call takes ~0.08s - this is huge! It would be better to check if ancestor respond to this method before calling it:

puts Benchmark.measure {
  ActiveRecord::Base.ancestors.each do |ancestor|
    next unless ancestor.respond_to?(:attachment_definitions)
    # call ancestor.attachment_definitions
  end
}
  0.000000   0.000000   0.000000 (  0.000169)

Also there is no need to call keys method in setup_attachment_class method. attachment_definitions returns hash and you can call has_key? method directly on hash without creating intermediate Arra (also Hash#has_key? has O(1) complexity and Array#member? O(N)).

@radarek radarek referenced a pull request that will close this issue Feb 23, 2015

Open

Optimize Database#setup_attachment_class method #17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment