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

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 · May be fixed by #17
Open

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

radarek opened this issue Feb 21, 2015 · 0 comments · May be fixed by #17

Comments

@radarek
Copy link

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 linked a pull request Feb 23, 2015 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant