Bug #11901
closedPerformance Issue with OpenStruct
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
Updated by amcaplan (Ariel Caplan) almost 9 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 9 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) almost 9 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) almost 9 years ago
- Backport set to 2.3: REQUIRED
Updated by naruse (Yui NARUSE) almost 9 years ago
- Status changed from Open to Closed
Could you fix rubyspec?
Updated by marcandre (Marc-Andre Lafortune) almost 9 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) almost 9 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 redefinefreeze
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 8 years ago
- Backport changed from 2.3: REQUIRED to 2.3: DONE
ruby_2_3 r54388 merged revision(s) 53395,53396.