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

Model filter unnecessarily re-constantizes strings #116

Closed
tfausak opened this issue Jan 27, 2014 · 1 comment
Closed

Model filter unnecessarily re-constantizes strings #116

tfausak opened this issue Jan 27, 2014 · 1 comment
Assignees
Milestone

Comments

@tfausak
Copy link
Collaborator

tfausak commented Jan 27, 2014

The model filter spends a lot of time repeating work it's already done.

filter1 = ActiveInteraction::ModelFilter.new(:object)
filter2 = ActiveInteraction::ModelFilter.new(:object, class: Object)
filter3 = ActiveInteraction::ModelFilter.new(:object, class: :object)
filter4 = ActiveInteraction::ModelFilter.new(:object, class: 'Object')

n = 10_000
object = Object.new

Benchmark.bmbm do |bm|
  bm.report { n.times { filter1.cast(object) } } # => 0.235544
  bm.report { n.times { filter2.cast(object) } } # => 0.254362
  bm.report { n.times { filter3.cast(object) } } # => 0.253539
  bm.report { n.times { filter4.cast(object) } } # => 0.249165
end

Granted, that's not too slow. But it can be much, much faster. Caching the class speeds this up by two orders of magnitude.

module ActiveInteraction
  class ModelFilter < Filter
    def klass
      return @klass if instance_variable_defined?(:@klass)

      klass_name = options.fetch(:class, name).to_s.classify
      @klass ||= klass_name.constantize
    rescue NameError
      raise InvalidClassError, klass_name.inspect
    end
  end
end
  bm.report { n.times { filter1.cast(object) } } # => 0.002667
  bm.report { n.times { filter2.cast(object) } } # => 0.002707
  bm.report { n.times { filter3.cast(object) } } # => 0.005106
  bm.report { n.times { filter4.cast(object) } } # => 0.001633

I think we decided to naively re-constantize it every time to avoid something like cypriss/mutations#23.

@tfausak
Copy link
Collaborator Author

tfausak commented Jan 27, 2014

This is related to #91.

tfausak added a commit that referenced this issue Jan 28, 2014
This gives us the performance benefit we want without introducing a bug
or requiring a configuration option.

I am a little concerned that an infinite loop might be possible. The
only way I can think of that happening is if a class overrides `.name`
to provide something other than the class name.
@ghost ghost assigned tfausak Jan 28, 2014
@tfausak tfausak closed this as completed Jan 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant