lsegal
commented
10 months ago In order to consider this a defect worth fixing (and possibly causing a backwards incompatible change to the API), there would need to be more-than-theoretical / real world data from YARD usage. As shown later in this post, I cannot reproduce such a slowdown. It's worth noting that the slow OpenStruct behavior is in first-write on an attribute in newly-allocated struct objects. Below is a much more comprehensive benchmark of the behavior, much more complete than the benchmarks linked above and illustrates the real issue.
Complete OpenStruct benchmarks
### Benchmark code: ```ruby require 'benchmark' require 'ostruct' n = 100000 class MyStruct < Struct.new(:a, :b, :c); end ostruct = OpenStruct.new mystruct = MyStruct.new Benchmark.bmbm do |x| x.report("OpenStruct.new(args)") { n.times { OpenStruct.new a: 1, b: 2, c: 3 } } x.report("Struct.new(args)") { n.times { MyStruct.new 1, 2, 3 } } x.report("OpenStruct.new (blank)") { n.times { OpenStruct.new } } x.report("OpenStruct (assign)") { n.times { ostruct.a = 1 } } x.report("Struct (assign)") { n.times { mystruct.a = 1 } } x.report("OpenStruct (read)") { n.times { ostruct.a } } x.report("Struct (read)") { n.times { mystruct.a } } end ``` ### Results: ``` Rehearsal ---------------------------------------------------------- OpenStruct.new(args) 1.469000 0.016000 1.485000 ( 1.477508) Struct.new(args) 0.031000 0.000000 0.031000 ( 0.017986) OpenStruct.new (blank) 0.016000 0.000000 0.016000 ( 0.017839) OpenStruct (assign) 0.016000 0.000000 0.016000 ( 0.012422) Struct (assign) 0.000000 0.000000 0.000000 ( 0.004586) OpenStruct (read) 0.015000 0.000000 0.015000 ( 0.010191) Struct (read) 0.000000 0.000000 0.000000 ( 0.004275) ------------------------------------------------- total: 1.563000sec user system total real OpenStruct.new(args) 1.485000 0.000000 1.485000 ( 1.471337) Struct.new(args) 0.015000 0.000000 0.015000 ( 0.016888) OpenStruct.new (blank) 0.016000 0.000000 0.016000 ( 0.018453) OpenStruct (assign) 0.015000 0.000000 0.015000 ( 0.011916) Struct (assign) 0.000000 0.000000 0.000000 ( 0.004551) OpenStruct (read) 0.016000 0.000000 0.016000 ( 0.009480) Struct (read) 0.015000 0.000000 0.015000 ( 0.004149) ```In practice, YARD doesn't really go through this "slow" code path often because most OpenStruct objects long-lived and shared, so first-write of attributes is fairly rare (we use structs to store shared state while parsing entire source trees creating maybe only a handful of structs). There are a few edge cases here-- if you use a lot of macros / directives, for example, you could see an impact in performance.
That said, changing this would be a breaking change to YARD's API, since OpenStructs are part of the public extension API in order to allow developers to provide custom context while parsing. It is not possible to exchange OpenStruct with Struct/Data-- they simply do not do the same thing. In order to provide similar (but not exactly equal) support to OpenStruct, we would need to implement a custom OpenStruct class that basically does the same thing as OpenStruct itself. I wrote up a quick example of this as YARD::OpenStruct and added it to YARD's codebase (with passing tests) and the change was not detectable in a full run of yard on YARD's own directory structure:
# CURRENT BEHAVIOR
$ time ruby bin/yard
Files: 186
Modules: 49 ( 6 undocumented)
Classes: 209 ( 41 undocumented)
Constants: 98 ( 29 undocumented)
Attributes: 179 ( 0 undocumented)
Methods: 997 ( 136 undocumented)
86.16% documented
real 0m15.722s
user 0m0.015s
sys 0m0.000s
# AFTER YARD::OpenStruct patch:
$ time ruby bin/yard
Files: 186
Modules: 49 ( 6 undocumented)
Classes: 209 ( 41 undocumented)
Constants: 98 ( 29 undocumented)
Attributes: 179 ( 0 undocumented)
Methods: 997 ( 136 undocumented)
86.16% documented
real 0m15.653s
user 0m0.000s
sys 0m0.000sNote that the above is just one sample, the averages range across the board and the tl;dr is that this change would not improve performance in any noticeable way. For reference, the YARD::OpenStruct implementation is provided below with benchmarks to compare to OpenStruct/Struct:
Full YARD::OpenStruct benchmarks
### Implementation / benchmark code: ```ruby require 'benchmark' require 'ostruct' module YARD # An OpenStruct compatible struct class that allows for basic access of attributes # via +struct.attr_name+ and +struct.attr_name = value+. class OpenStruct def initialize(hash = {}) hash.each { |k, v| instance_variable_set("@#{k}", v) } end # @private def method_missing(name, *args) if name =~ /=$/ instance_variable_set("@#{name.to_s[0..-2]}", args.first) else instance_variable_get("@#{name.to_s}") end end def to_h instance_variables.each_with_object({}) do |var, hash| hash[var.to_s[1..-1].to_sym] = instance_variable_get(var) end end def ==(other) other.is_a?(self.class) && to_h == other.to_h end end end n = 100000 class MyStruct < Struct.new(:a, :b, :c); end ostruct = OpenStruct.new yostruct = YARD::OpenStruct.new mystruct = MyStruct.new Benchmark.bmbm do |x| x.report("Struct.new(args)") { n.times { MyStruct.new 1, 2, 3 } } x.report("Struct (assign)") { n.times { mystruct.a = 1 } } x.report("Struct (read)") { n.times { mystruct.a } } x.report("OpenStruct.new(args)") { n.times { OpenStruct.new a: 1, b: 2, c: 3 } } x.report("OpenStruct.new (blank)") { n.times { OpenStruct.new } } x.report("OpenStruct (assign)") { n.times { ostruct.a = 1 } } x.report("OpenStruct (read)") { n.times { ostruct.a } } x.report("YARD::OpenStruct.new(args)") { n.times { YARD::OpenStruct.new a: 1, b: 2, c: 3 } } x.report("YARD::OpenStruct.new (blank)") { n.times { YARD::OpenStruct.new } } x.report("YARD::OpenStruct (assign)") { n.times { yostruct.a = 1 } } x.report("YARD::OpenStruct (read)") { n.times { yostruct.a } } end ``` ### Results: ``` Rehearsal ---------------------------------------------------------------- Struct.new(args) 0.031000 0.000000 0.031000 ( 0.019441) Struct (assign) 0.000000 0.000000 0.000000 ( 0.005331) Struct (read) 0.000000 0.000000 0.000000 ( 0.004483) OpenStruct.new(args) 1.578000 0.000000 1.578000 ( 1.547479) OpenStruct.new (blank) 0.032000 0.000000 0.032000 ( 0.018671) OpenStruct (assign) 0.015000 0.000000 0.015000 ( 0.013305) OpenStruct (read) 0.047000 0.000000 0.047000 ( 0.009657) YARD::OpenStruct.new(args) 0.110000 0.000000 0.110000 ( 0.106746) YARD::OpenStruct.new (blank) 0.015000 0.000000 0.015000 ( 0.019997) YARD::OpenStruct (assign) 0.078000 0.000000 0.078000 ( 0.086861) YARD::OpenStruct (read) 0.016000 0.000000 0.016000 ( 0.004378) ------------------------------------------------------- total: 1.922000sec user system total real Struct.new(args) 0.015000 0.000000 0.015000 ( 0.017003) Struct (assign) 0.000000 0.000000 0.000000 ( 0.004562) Struct (read) 0.000000 0.000000 0.000000 ( 0.004176) OpenStruct.new(args) 1.500000 0.000000 1.500000 ( 1.510916) OpenStruct.new (blank) 0.031000 0.000000 0.031000 ( 0.017657) OpenStruct (assign) 0.000000 0.000000 0.000000 ( 0.011936) OpenStruct (read) 0.016000 0.000000 0.016000 ( 0.009653) YARD::OpenStruct.new(args) 0.093000 0.000000 0.093000 ( 0.107001) YARD::OpenStruct.new (blank) 0.016000 0.000000 0.016000 ( 0.019866) YARD::OpenStruct (assign) 0.016000 0.000000 0.016000 ( 0.004695) YARD::OpenStruct (read) 0.000000 0.000000 0.000000 ( 0.004348) ```As you can see, the naive implementation I threw together in a few minutes fixes the fundamental allocation speed issue-- but it changes the API. YARD extension authors who were relying on various OpenStruct methods would start seeing failures, and in order to make this backward compatible, YARD would be on the hook for implementing a drop-in replacement to OpenStruct just to fix an allocation performance issue which could have been patched in OpenStruct itself.
This gets to the heart of the issue: the idea that OpenStruct "is slow" seems like an entirely bogus claim based on bad data. I could be off base here since I haven't investigated the current OpenStruct implementation too closely, but I was able to throw together a reasonably performant replacement in a few minutes, it seems entirely probable that OpenStruct's own allocation issue could have been fixed instead of providing the warning that the Ruby core team chose to add.
I would say that this should be marked as an upstream defect. Why would we just fix this in YARD when the Ruby team could be addressing the performance issue and fixing it for everybody? Especially given that in practice, this does not affect actual performance in YARD, but likely does in other Ruby applications.
bkuhlmann
Hello. :wave: I'm seeing performance warnings show up when using YARD in Ruby 3.3.0 due to heavy use of
OpenStructwhich the Ruby core team no longer recommends being used.Steps to reproduce
You'll want to ensure that performance warnings are enabled. Example:
With the above in place, now you can recreate using either of the following:
`yard doc --no-private`
``` /Users/bkuhlmann/.cache/frum/versions/3.3.0/lib/ruby/gems/3.3.0/gems/yard-0.9.34/lib/yard/templates/template_options.rb:34: warning: OpenStruct use is discouraged for performance reasons /Users/bkuhlmann/.cache/frum/versions/3.3.0/lib/ruby/gems/3.3.0/gems/yard-0.9.34/lib/yard/parser/source_parser.rb:365: warning: OpenStruct use is discouraged for performance reasons /Users/bkuhlmann/.cache/frum/versions/3.3.0/lib/ruby/gems/3.3.0/gems/yard-0.9.34/lib/yard/templates/template_options.rb:34: warning: OpenStruct use is discouraged for performance reasons Files: 0 Modules: 0 ( 0 undocumented) Classes: 0 ( 0 undocumented) Constants: 0 ( 0 undocumented) Attributes: 0 ( 0 undocumented) Methods: 0 ( 0 undocumented) 100.00% documented ``` Notice the performance warnings shown above.`gem install faker`
``` Successfully installed faker-3.2.2 Building YARD (yri) index for faker-3.2.2... /Users/bkuhlmann/.cache/frum/versions/3.3.0/lib/ruby/gems/3.3.0/gems/yard-0.9.34/lib/yard/templates/template_options.rb:34: warning: OpenStruct use is discouraged for performance reasons /Users/bkuhlmann/.cache/frum/versions/3.3.0/lib/ruby/gems/3.3.0/gems/yard-0.9.34/lib/yard/parser/source_parser.rb:365: warning: OpenStruct use is discouraged for performance reasons /Users/bkuhlmann/.cache/frum/versions/3.3.0/lib/ruby/gems/3.3.0/gems/yard-0.9.34/lib/yard/parser/source_parser.rb:365: warning: OpenStruct use is discouraged for performance reasons /Users/bkuhlmann/.cache/frum/versions/3.3.0/lib/ruby/gems/3.3.0/gems/yard-0.9.34/lib/yard/handlers/processor.rb:101: warning: OpenStruct use is discouraged for performance reasons /Users/bkuhlmann/.cache/frum/versions/3.3.0/lib/ruby/gems/3.3.0/gems/yard-0.9.34/lib/yard/docstring_parser.rb:90: warning: OpenStruct use is discouraged for performance reasons ``` Notice the performance warnings displayed when installing the gem. What's shown above is only a small example, this actually repeats for several pages.Actual Output
See above.
Expected Output
I would expect to see no performance warnings due to the removal of
OpenStructsince it has terrible performance. If you need benchmarks, check out howOpenStructperforms ~4,000 times worse than aStructorDataobject in these benchmarks.Environment details: