Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix warning: ostruct was loaded from the standard library #363

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

taketo1113
Copy link

It fixes a warning of loading ostruct gem from standard library with Ruby 3.3.5 and 3.4+.

The warning message is following:

/path/config/lib/config.rb:1: warning: ostruct was loaded from the standard library, but will no longer be part of the default gems starting from Ruby 3.5.0.
You can add ostruct to your Gemfile or gemspec to silence this warning.
  • ruby: 3.3.5
  • config gem: 5.5.1

Related Links

Additional information

This warning category is performance.
It may be better to reduce the dependency of ostruct.

OpenStruct use is discouraged for performance reasons

ruby/ostruct#56

config.gemspec Outdated
@@ -28,6 +28,7 @@ Gem::Specification.new do |s|
s.required_ruby_version = '>= 2.6.0'

s.add_dependency 'deep_merge', '~> 1.2', '>= 1.2.1'
s.add_dependency 'ostruct' if RUBY_VERSION >= '3.3.5'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you can do conditionals like this in a gemspec, because when you gem package it evaluates this at package time. That is, if it's packaged on < 3.3.5 it won't exist, and if it's packaged on 3.3.5+ it will exist, which means it's non-deterministic.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fryguy Thank you for a comment.
I removed condition.

@Fryguy
Copy link
Member

Fryguy commented Sep 6, 2024

It may be better to reduce the dependency of ostruct.

ostruct is really a core component of this gem, because that's what provides the dot access to all the settings.

What makes ostruct a performance issue is when you have to create a lot of them only to access their settings a small amount of times (high create-low read/write), because ostruct creates a lot of method accessors on the singleton object under the covers. The overhead of creating those method accessors outweighs the time to access the underlying value. A popular way to do it differently is to use something like method missing, but that flips the performance issue. If you have to access a particular value a lot of times on a cached/singleton object, the overhead of the extra method_missing dispatch outweighs the method creation

In general, Config falls into the latter category. When you start your app, it creates all the ostructs, and then that's done, generally for the lifetime of the app, and you can access as many times as your app needs. I would guess most app have a very high read rate if they have do it in every request, for example.

So, if we wanted to drop ostruct we could easily replace ostruct with something like method_missing, but that would actually likely introduce a new performance problem on the high-read side. Otherwise we'd have to duplicate ostruct's ability to create method accessors, which probably wouldn't be too bad, but feels like overhead when there's a gem that does that.

@taketo1113
Copy link
Author

@Fryguy As you say, there is no need to replace ostruct.

I created PoC to compare ostruct with method_missing.
The benchmark result is below.
It comfirm a new performance problem on the high-read side with MethodMissingClass (assign/read).

                                            user     system      total        real
OpenStruct.new (without args)           0.001136   0.000023   0.001159 (  0.001158)
MethodMissingClass.new (without args)   0.000699   0.000004   0.000703 (  0.000702)
OpenStruct (assign)                     0.000655   0.000001   0.000656 (  0.000657)
MethodMissingClass (assign)             0.002662   0.000009   0.002671 (  0.002674)
OpenStruct (read)                       0.000727   0.000000   0.000727 (  0.000727)
MethodMissingClass (read)               0.002228   0.000021   0.002249 (  0.002248)
PoC & Benchmark

PoC & Benchmark code

# benchmark.rb
require 'benchmark'
require 'ostruct'

puts RUBY_DESCRIPTION

n = 10_000
puts "times: #{n}"

# OpenStruct
ostruct = OpenStruct.new

# method_missing
class MethodMissingClass
  def method_missing(name, *value)
    self.class.class_eval do
      define_method "#{name}" do |*value|
        if name.end_with?('=')
          instance_variable_set("@#{name.to_s.chop}", *value)
        else
          instance_variable_get("@#{name}")
        end
      end
    end
    send(name, *value)
  end
end
method_missing = MethodMissingClass.new

# Benchmark
Benchmark.bmbm do |x|
  x.report("OpenStruct.new (without args)") { n.times { OpenStruct.new } }
  x.report("MethodMissingClass.new (without args)") { n.times { MethodMissingClass.new } }
  x.report("OpenStruct (assign)") { n.times { ostruct.a = 1 } }
  x.report("MethodMissingClass (assign)") { n.times { method_missing.a = 1 } }
  x.report("OpenStruct (read)") { n.times { ostruct.a } }
  x.report("MethodMissingClass (read)") { n.times { method_missing.a } }
end

Benchmark Result:

$ ruby --yjit benchmark_method_missing.rb
ruby 3.3.5 (2024-09-03 revision ef084cc8f4) +YJIT [arm64-darwin23]
times: 10000
Rehearsal -------------------------------------------------------------------------
OpenStruct.new (without args)           0.001591   0.000190   0.001781 (  0.001780)
MethodMissingClass.new (without args)   0.000912   0.000011   0.000923 (  0.000923)
OpenStruct (assign)                     0.000753   0.000028   0.000781 (  0.000781)
MethodMissingClass (assign)             0.003194   0.000066   0.003260 (  0.003263)
OpenStruct (read)                       0.000826   0.000021   0.000847 (  0.000848)
MethodMissingClass (read)               0.002592   0.000057   0.002649 (  0.002652)
---------------------------------------------------------------- total: 0.010241sec

                                            user     system      total        real
OpenStruct.new (without args)           0.001136   0.000023   0.001159 (  0.001158)
MethodMissingClass.new (without args)   0.000699   0.000004   0.000703 (  0.000702)
OpenStruct (assign)                     0.000655   0.000001   0.000656 (  0.000657)
MethodMissingClass (assign)             0.002662   0.000009   0.002671 (  0.002674)
OpenStruct (read)                       0.000727   0.000000   0.000727 (  0.000727)
MethodMissingClass (read)               0.002228   0.000021   0.002249 (  0.002248)

@pkuczynski
Copy link
Member

@taketo1113 updating changelog would be useful

@taketo1113
Copy link
Author

@pkuczynski Currently, CHANGELOG does not have a section of Unreleased.
Could I add a section of Unreleased at CHANGELOG?
https://github.com/rubyconfig/config/blob/86c47f25f3ed1eb29827bfcf6f1a30d811a964b7/CHANGELOG.md

@pkuczynski
Copy link
Member

@pkuczynski Currently, CHANGELOG does not have a section of Unreleased. Could I add a section of Unreleased at CHANGELOG? https://github.com/rubyconfig/config/blob/86c47f25f3ed1eb29827bfcf6f1a30d811a964b7/CHANGELOG.md

Yes please...

@taketo1113
Copy link
Author

@pkuczynski I updated CHANGELOG.

@pkuczynski
Copy link
Member

@Fryguy @cjlarose are you happy with those changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants