method_missing for helpers is slow

[gurgeous]

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?

[jamesbrooks]

@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!

[gurgeous]

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