method_missing for helpers is slow
Opened this issue · 2 comments
Hi there, thanks for creating such a great project!
We noticed that a particular request in our Rails app was slow. Eventually we traced the problem to a decorator that was calling image_tag
. Each call results in a method_missing
through ActiveDecorator, which delegates to the view context. The overhead for each call is around 1ms, and we are calling image_tag 100 times, so the decorator was slowing down the page by 100ms.
Not sure about the proper fix. Can we mix in the helpers directly somehow, instead of relying on method_missing & view context? Should we call define_method
to add missing methods as they are detected? Should we expose view_context
and force decorators to call it directly?
@gurgeous Interesting issue and always interesting when some small time use adds up quickly over time, 100ms is nothing to scoff at and worth eliminating from production if possible.
Have you made any headway on this issue in the past two and a bit weeks? My initial thought reading the problem was one potential solution you described which was using define_method
within method_missing
to get a more responsive invocation.
Would be interested to hear how you're getting on with this as a potential solution should be able to be authored in a fairly generic way I imagine for wide-spread consumption - I wonder if I've got lots of dead-time in my application as a result of something similar!
I ended up mixing in a module that prints warnings on excess method_missing calls and exposes view_context
. It also lazily delegates hot methods like image_tag
. We couldn't delegate normally because some of the offending methods conflict with method names from the models.
Here's the code in case anyone finds it useful. Note that this depends on some other gems like request_store, rainbow, etc.
#
# ActiveDecorator uses method_missing to resolve naked helper methods. This is
# slow. It's not a huge problem, but if you call dozens of these it will affect
# response times. For example, two naked helper calls were slowing down one
# of our pages by 100ms for some users.
#
# The fix is to add "view_context." in front of the helper method or add to the
# delegate call below. We also instrument ActiveDecorator to log problematic
# calls to helper methods.
#
# In addition, we delegate a few common methods to view_context. This is done
# lazily to avoid interfering with associations, stubbing of current_user in tests,
# manually including NumberHelper, etc.
#
# https://github.com/amatsuda/active_decorator/blob/master/lib/active_decorator/helpers.rb
# https://github.com/amatsuda/active_decorator/issues/114
#
module ActiveDecoratorMethodMissing
REQUEST_STORE_KEY = :admm
# Methods in this list will be lazily delegated to view_context if they are
# found to be missing. This automatically fixes the method missing penalty
# after the first miss.
LAZILY_DELEGATE = %i[
current_user
image_tag
link_to
number_with_delimiter
tag
].to_set.freeze
# expose this so decorators can fix the issue manually
def view_context
ActiveDecorator::ViewContext.current
end
# rubocop:disable all
def method_missing(method, *args, &block)
# skip super, call super.super instead
method(:method_missing).super_method.super_method.call(method, *args, &block)
rescue NoMethodError, NameError => e
# the error is not mine, so just releases it as is.
raise e if e.name != method
if (view_context = ActiveDecorator::ViewContext.current)
# tally for log_warnings
(RequestStore.store[REQUEST_STORE_KEY] ||= Hash.new(0))[method] += 1
begin
view_context.send(method, *args, &block).tap do
# we successfully delegated to view_context. Make it automatic to
# avoid the method missing penalty next time.
if LAZILY_DELEGATE.include?(method)
self.class.delegate(method, to: :view_context)
end
end
rescue NoMethodError => e
raise e if e.name != method
raise NoMethodError.new("undefined method `#{method}' for either #{self} or #{view_context}", method)
rescue NameError => e
raise e if e.name != method
raise NameError.new("undefined local variable `#{method}' for either #{self} or #{view_context}", method)
end
else # AC::API would have not view_context
raise e
end
end
# rubocop:enable all
# calls this from an after_filter in your controller (in dev)
def self.log_warnings
tally = RequestStore.store[REQUEST_STORE_KEY]
return if !tally
excessive = tally.select { _2 >= 20 }.sort.to_h
return if excessive.blank?
s = " ActiveDecorator helpers called excessively without view_context: #{excessive}"
Rails.logger.warn(Rainbow(s).background(:red).bold.white)
end
end
::ActiveDecorator::Helpers.include ActiveDecoratorMethodMissing