Project

General

Profile

Bug #11901

Performance Issue with OpenStruct

Added by amcaplan (Ariel Caplan) almost 5 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Normal
Target version:
-
ruby -v:
ruby 2.3.0p0 (2015-12-25 revision 53290) [x86_64-darwin13]
Backport:
[ruby-core:72523]

Description

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.


Files

openstruct-regression-benchmark.rb (1.36 KB) openstruct-regression-benchmark.rb Benchmark to compare 2.2.4 versus 2.3.0 amcaplan (Ariel Caplan), 12/27/2015 03:35 PM

Updated by amcaplan (Ariel Caplan) almost 5 years ago

  • 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.

Updated by amcaplan (Ariel Caplan) almost 5 years ago

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?

Updated by marcandre (Marc-Andre Lafortune) over 4 years ago

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.

Updated by marcandre (Marc-Andre Lafortune) over 4 years ago

  • Backport set to 2.3: REQUIRED

Updated by naruse (Yui NARUSE) over 4 years ago

  • Status changed from Open to Closed

Could you fix rubyspec?

Updated by marcandre (Marc-Andre Lafortune) over 4 years ago

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.

Updated by Eregon (Benoit Daloze) over 4 years ago

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.

Updated by naruse (Yui NARUSE) over 4 years ago

  • Backport changed from 2.3: REQUIRED to 2.3: DONE

ruby_2_3 r54388 merged revision(s) 53395,53396.

Also available in: Atom PDF