Shrine: Avoid string evals

[printercu]

Hi!

I think define_method is preferred way to define dynamic methods rather than eval'ing generated strings:

• static code analysis can be used with define_method,
• it's more debugger-friendly,
• syntax errors in evaled code can be found only when code is executed,
• it's usually easier to read define_methods (and sometimes easier to write).

I see that most of code which requires evaling strings is used for interpolating #{name}_attacher. This can be avoided by using method shrine_attacher_for(name, options). I see 2 ways:

• make shrine_attacher_for the base one, and *_attacher methods will use it;
• keep *_attacher methods as base one, and define shrine_attacher_for as public_send("#{name}_attacher").

1st way requires class-level configuration registry which maps attachment name to attacher class and takes more effort to implement, while brings new features and can be as fast as current implementation. 2nd way requires minimal efforts but is slightly slower then current implementation. However this performance penalties are small enough.

Here is benchmark. Because extra method call is slower than no method call, I've added extracted basic implementation of core classes to get results in right scale.

Benchmark source

class Uploader
  def initialize(*)
    #
  end

  class Attacher
    @shrine_class = Uploader

    class << self
      attr_reader :shrine_class
    end

    def initialize(record, name, cache: :cache, store: :store)
      @cache   = shrine_class.new(cache)
      @store   = shrine_class.new(store)
      @context = {record: record, name: name}
      @errors  = []
    end

    def shrine_class
      self.class.shrine_class
    end
  end

  module Attachment
    @options = {}
    @name = :image
    @shrine_class = Uploader

    class << self
      attr_reader :shrine_class

      def build_attacher(instance, options)
        shrine_class::Attacher.new(instance, @name, @options.merge(options))
      end
    end

    attachment    = self
    attacher_ivar = :"@#{@name}_attacher"
    define_method :"#{@name}_attacher" do |options = {}|
      if !instance_variable_get(attacher_ivar) || options.any?
        instance_variable_set(attacher_ivar, attachment.build_attacher(self, options))
      else
        instance_variable_get(attacher_ivar)
      end
    end

    def attacher_for(name)
      public_send("#{name}_attacher")
    end

    def attacher_for_cached(name, options = nil)
      @attachers ||= {}
      if !options || options.empty?
        @attachers[name] ||= public_send("#{name}_attacher")
      else
        @attachers[name] = public_send("#{name}_attacher")
      end
    end
  end
end

class Model
  include Uploader::Attachment
end

require 'benchmark/ips'

puts 'first call'
Benchmark.ips do |x|
  x.report('direct') { Model.new.image_attacher }
  x.report('indirect') { Model.new.attacher_for(:image) }
  x.report('indirect cached') { Model.new.attacher_for_cached(:image) }
  x.compare!
end

puts nil, 'subsequent'
model = Model.new.tap(&:image_attacher)
Benchmark.ips do |x|
  x.report('direct') { model.image_attacher }
  x.report('indirect') { model.attacher_for(:image) }
  x.report('indirect cached') { model.attacher_for_cached(:image) }
  x.compare!
end

first call
Warming up --------------------------------------
              direct    18.645k i/100ms
            indirect    16.966k i/100ms
     indirect cached    14.317k i/100ms
Calculating -------------------------------------
              direct    200.277k (± 9.6%) i/s -    988.185k in   5.005787s
            indirect    184.168k (± 3.9%) i/s -    933.130k in   5.076558s
     indirect cached    156.482k (± 6.0%) i/s -    787.435k in   5.054927s

Comparison:
              direct:   200276.6 i/s
            indirect:   184167.6 i/s - same-ish: difference falls within error
     indirect cached:   156481.8 i/s - 1.28x  slower


subsequent
Warming up --------------------------------------
              direct   148.417k i/100ms
            indirect    93.910k i/100ms
     indirect cached   181.344k i/100ms
Calculating -------------------------------------
              direct      2.697M (± 4.9%) i/s -     13.506M in   5.022511s
            indirect      1.340M (± 5.9%) i/s -      6.762M in   5.067422s
     indirect cached      3.982M (± 3.6%) i/s -     19.948M in   5.018257s

Comparison:
     indirect cached:  3981536.3 i/s
              direct:  2696916.9 i/s - 1.48x  slower
            indirect:  1340370.1 i/s - 2.97x  slower

In first call difference (indirect cached - direct) = 1.4 μ sec per call, in subsequent call (indirect - direct) = 0.38 μ sec/per call.

WDYT? I can add this method to Attachment and change all evals to define_method.

[janko]

Thanks for the thorough research. Now that we've switched the #{name}_attacher method to define_method, I think I would also prefer to switch other methods too, for consistency.

However, I wouldn't go as far as reducing the number of method calls, as I don't think it's worth it, so I would prefer if other methods just keep calling the #{name}_attacher method. For that we don't need the #attacher_for method (and I don't think that's a good idea, because it's not attachment-specific, so if a model includes multiple attachments, the methods will override each other).

This is the approach I would prefer:

def define_attachment_methods!
  attachment = self
  name       = self.attachment_name

  define_method :"#{name}_attacher" do |options = {}|
    if !instance_variable_get(:"@#{name}_attacher") || options.any?
      instance_variable_set(:"@#{name}_attacher", attachment.build_attacher(self, options))
    else
      instance_variable_get(:"@#{name}_attacher")
    end
  end

  define_method :"#{name}=" do |value|
    send(:"#{name}_attacher").assign(value)
  end

  define_method :"#{name}" do |value|
    send(:"#{name}_attacher").get
  end

  define_method :"#{name}_url" do |*args|
    send(:"#{name}_attacher").url(*args)
  end
end

What do you think? If you agree and would like to make a PR, could you also make sure other plugins which add additional attachment methods are also switched to using define_method?

[jrochkind]

I honestly like the readability of the present implementation. It's possible a new implementation can be just as readable (or even more?). But I don't think we need to sacrifice any legibility for micro-optimizations of performance at this level, I think readability should probably be the primary goal here.

[printercu]

I think that less dynamic calls is better (not by number of invocations, but by number of uses in source): it's more readable, all invocations can be searched easily (comparing to interpolations: some may be send(:"#{name}_attacher"), others with send(:"#{other_name}_attacher")), it bring's single entry-point.

define_method :"#{name}=" do |value|
  send(:"#{name}_attacher").assign(value)
end

define_method :"#{name}_url" do |*args|
  send(:"#{name}_attacher").url(*args)
end

####

define_method :"#{name}=" do |value|
  shrine_attacher_for(name).assign(value)
end

define_method :"#{name}_url" do |*args|
  shrine_attacher_for(name).url(*args)
end

_Offtopic_. Same for repetitive :"@#{name}_attacher". Extracting this to local variable prevents from need to reed every this string carefully, looking for differences. With tiny bonus of avoiding concatenations and conversion string -> symbol 2 times on each call.

so if a model includes multiple attachments, the methods will override each other

This can be avoided by

class Attachment < Module
  @shrine_class = ::Shrine

  module CommonMethods
    def shrine_attachment_for(*)
      #
    end
  end

  module InstanceMethods
    def define_attachment_methods!
      include CommonMethods

      define_method #...
    end
  end
end

It's just a my opinion. If you think that send(:"#{name}_attacher") is better, I still think that this is improvement for current implementation.

[printercu]

@jrochkind changing string evals to define_methods is not about performance optimizations (because define methods can even will be insignificantly slower). Here are pros from my first comment:

• static code analysis can be used with define_method,
• it's more debugger-friendly,
• syntax errors in evaled code can be found only when code is executed,
• it's usually easier to read define_methods (and sometimes easier to write).

About the readability. In this example it's obvious for me that evals are less readable:

model.class_eval <<-RUBY, __FILE__, __LINE__ + 1 if opts[:activerecord_validations]
  validate do
    #{@name}_attacher.errors.each do |message|
      errors.add(:#{@name}, *message)
    end
  end
RUBY

#### 

if opts[:activerecord_validations]
  model.validate do
    shrine_attacher_for(name).errors.each do |message|
      errors.add(name, *message)
    end
  end
end

[hmistry]

@printercu I'm with @jrochkind when it comes to readability - the present method has been easy to work with and has never been an issue with debugging or syntax errors or any one of the listed pro's. This is a personal preference for sure and if you're new there's always the knee-jerk reactions to different styles and wrapping the head around a new code base. The real question is after that point if the code is still unreadable or hard to work with - and for us I can safely say it hasn't been an issue.

With that said, I open to @janko-m proposal above of using define_method as it's still similar enough to the existing code but let's not make it any more complex than it needs to be.

I do have one requirement should we decide to make the change and that is I'd like to see a memory allocation and GC benchmark with a real AR models before and after to ensure that everything is being GC'd as it should be with define_method. I'm not sure if Ruby has changed it's behavior since 2013 and to what extent define_method affects garbage collection rules when we're including modules in AR models but the example below concerns me if an AR model has other large objects that will no longer be GC'd because they added a Shrine::Attachment.

# The main down side is that define_method creates a closure. The closure could hold 
# references to large objects, and those large objects will never be garbage collected.

class Foo
  x = "X" * 1024000   # Not GC'd
  define_method("foo") { }
end

class Bar
  x = "X" * 1024000   # Is GC'd
  class_eval("def foo; end")
end

Reference: https://tenderlovemaking.com/2013/03/03/dynamic_method_definitions.html

[janko]

define_method :"#{name}=" do |value|
  send(:"#{name}_attacher").assign(value)
end

define_method :"#{name}_url" do |*args|
  send(:"#{name}_attacher").url(*args)
end

####

define_method :"#{name}=" do |value|
  shrine_attacher_for(name).assign(value)
end

define_method :"#{name}_url" do |*args|
  shrine_attacher_for(name).url(*args)
end

The downside of this approach for me is that it adds another level of indirection. Instead of having image= => image_attacher, you now have image= => shrine_attacher_for => image_attacher. Given that the image_attacher method needs to exist, I don't see any reason why we wouldn't call it directly.

Also, as I said, for me it doesn't feel right to add the #shrine_attacher_for method that's not attachment-specific (it doesn't have the attachment name in the method's name), as if you're including multiple attachment modules in a model, the attachment modules will override that method between each other (because they all define it). It wouldn't cause any actual problems, it just doesn't feel right from the design perspective.

Offtopic. Same for repetitive :"@#{name}_attacher". Extracting this to local variable prevents from need to reed every this string carefully, looking for differences. With tiny bonus of avoiding concatenations and conversion string -> symbol 2 times on each call.

I don't believe the reader will go look for differences. For me it's easier to read #define_attachment_methods! when there is one local variable which is used everywhere – name – than to introduce a second local variable. For me my suggestion spells out better what it's called, you don't have to look elsewhere. The chances are that when people see name, they will know what it means from context, but when they see attacher_ivar or something like that, they will likely go look for it to see what it holds.

We probably have different reading preferences, but for me "more DRY" doesn't always equal "more readable".

I do have one requirement should we decide to make the change and that is I'd like to see a memory allocation and GC benchmark with a real AR models before and after to ensure that everything is being GC'd as it should be with define_method

As I understood, only local variables are passed down to these blocks. In activerecord and sequel models the model class is also present as a local variable, which doesn't seem problematic for me as it has to be loaded anyway, since it's a constant. I don't think there is any reason for concern, but feel free to make a benchmark if you feel otherwise.

[printercu]

OK. Please clarify, if this is still wanted to replace eval <~RUBY with method calls? For me the main pros are

• avoid "dynamics" (runtime generated ruby code and method names). I hope this makes code simpler and easier for perception. I understand that this is biased, I think there is no researches on it yet :) Please evaluate this from the point of view of people not familiar with the code and are trying to resolve bug, or find out how the gem actually works under the hood.
• suitable for static analysis. But it's not used for the gem, and all generated methods are quite simple for now.

So this both may look insignificant. And I don't want to insist, if somebody prefers current option.

[janko]

@printercu Now that we switched #{name}_attacher to define_method, I wouldn't mind switching the others to define_method for consistency. I think that also makes it look less scary for people new to metaprogramming, as define_method is much milder than module_eval <<-RUBY, __FILE__, __LINE__ + 1.

[printercu]

I see there are a lot of methods that just delegates others to attacher like this:

          module_eval <<-RUBY, __FILE__, __LINE__ + 1
            def #{@name}_remote_url=(url)
              #{@name}_attacher.remote_url = url
            end

            def #{@name}_remote_url
              #{@name}_attacher.remote_url
            end
          RUBY

Would you mind if I add

      def delegate_to_attacher(methods)
        name = attachment_name
        methods.each do |method|
          define_method "#{name}_#{method}" do |*args, &block|
            send("#{name}_attacher").public_send(method, *args, &block)
          end
        end
      end

# usage
delegate_to_attacher :remote_url, :remote_url=

[janko]

This is one of those cases where I find the current version that's technically less DRY to be more readable. So I would prefer if we define each method individually.