Bug #11901: Performance Issue with OpenStruct

• Status: Closed
• Assignee: marcandre (Marc-Andre Lafortune)
• Target version: -
• ruby -v: ruby 2.3.0p0 (2015-12-25 revision 53290) [x86_64-darwin13]
• Backport: 2.3: DONE

[amcaplan (Ariel Caplan)]

After recent changes to define OpenStruct getter/setter methods lazily, there is a heavy performance impact for the use case where an attribute is assigned at initialization time (i.e. Openstruct.new(foo: :bar)). Once an attribute is stored in the internal hash, the appropriate singleton methods will never be defined, due to the recent changes to OpenStruct's #respond_to_missing? - meaning that every time I call #foo or #foo= it relies on #method_missing. Benchmark using benchmark-ips is attached.

I'm primarily concerned about the case of configuration objects, which may be populated at initialization time and then accessed many times throughout the life of the program.

[amcaplan (Ariel Caplan)]

• Assignee set to marcandre (Marc-Andre Lafortune)

To be more specific (but not clog up the description), the problem can be traced to https://github.com/ruby/ruby/blob/b8d9770b6c699af6e63dab727621777fbfbf7b44/lib/ostruct.rb#L166 where the methods are only defined if #respond_to? is false for that method name. Since #repond_to_missing? was overridden to report true when the key is in the table (https://github.com/ruby/ruby/blob/b8d9770b6c699af6e63dab727621777fbfbf7b44/lib/ostruct.rb#L176), the methods are never defined.

[amcaplan (Ariel Caplan)]

Now, to throw in my own opinion: probably the simplest fix would be to circumvent the #respond_to? check if we hit #method_missing? already - the check is both unnecessary and inaccurate. So probably we'd want the method defining methods to be its own method, and then have both #method_missing? and #new_ostruct_member rely on that. Something like:

class OpenStruct
  def new_ostruct_member(name)
    name = name.to_sym
    unless respond_to?(name)
      define_openstruct_methods(name)
    end
    name
  end

  def define_openstruct_methods(name)
    define_singleton_method(name) { @table[name] }
    define_singleton_method("#{name}=") { |x| modifiable[name] = x }
    name
  end

  def method_missing(mid, *args) # :nodoc:
    len = args.length
    if mname = mid[/.*(?==\z)/m]
      if len != 1
        raise ArgumentError, "wrong number of arguments (#{len} for 1)", caller(1)
      end
      modifiable[define_openstruct_methods(mname)] = args[0]
    elsif len == 0
      if @table.key?(mid)
        define_openstruct_methods(mid)
        @table[mid]
      end
    else
      err = NoMethodError.new "undefined method `#{mid}' for #{self}", mid, args
      err.set_backtrace caller(1)
      raise err
    end
  end
end

Running the previously attached benchmark prefaced with this code, the "assigned on initialization" benchmark outperforms the "assigned after initialization" one.

However, it does raise the question: Should calling #foo= define the methods actively, or should we only try to define on #foo to match lazy behavior on the initializer?

[marcandre (Marc-Andre Lafortune)]

Indeed, the test in new_ostruct_member was incorrect now that respond_to_missing? has been changed.

Instead, if we look for the actual method I believe this will fix all issues.

[marcandre (Marc-Andre Lafortune)]

• Backport set to 2.3: REQUIRED

[naruse (Yui NARUSE)]

• Status changed from Open to Closed

Could you fix rubyspec?

[marcandre (Marc-Andre Lafortune)]

Yui NARUSE wrote:

Could you fix rubyspec?

Great, that was a bug

I made an easy fix so method_missing doesn't add a method if frozen. I hesitated to redefine freeze to add all the needed method definitions, but it's difficult to know if it would be preferable.

[Eregon (Benoit Daloze)]

Marc-Andre Lafortune wrote:

Yui NARUSE wrote:

Could you fix rubyspec?

Great, that was a bug

I made an easy fix so method_missing doesn't add a method if frozen. I hesitated to redefine freeze to add all the needed method definitions, but it's difficult to know if it would be preferable.

nobu did the latter in r53396.

[naruse (Yui NARUSE)]

• Backport changed from 2.3: REQUIRED to 2.3: DONE

ruby_2_3 r54388 merged revision(s) 53395,53396.