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

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

taketo1113
Copy link
Contributor

@taketo1113 taketo1113 commented Sep 6, 2024

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

Closes #365

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
Contributor 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
Contributor 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
Contributor 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
Contributor Author

@pkuczynski I updated CHANGELOG.

@pkuczynski
Copy link
Member

@Fryguy @cjlarose are you happy with those changes?

@jsugarman
Copy link

This issue can be closed once merged 👍

@gstokkink
Copy link

@Fryguy can we get a release for this fix? 😄

@Fryguy
Copy link
Member

Fryguy commented Sep 20, 2024

I'd love to but I'm not a gem owner. @pkuczynski can do it once this is merged ☺️

@gstokkink
Copy link

gstokkink commented Sep 23, 2024

Ah, okay! @pkuczynski any chance of a release soon for this fix? Everyone upgrading to Ruby 3.3.5 or beyond will run into this warning, and for CIs that forbid deprecation warnings, this is kind of an issue 😋

@pkuczynski pkuczynski merged commit f083d95 into rubyconfig:master Sep 23, 2024
8 checks passed
@pkuczynski
Copy link
Member

Sure! Done.

@taketo1113 taketo1113 deleted the fix-waring-ostruct branch September 23, 2024 23:44
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.

Add "ostruct" to gemspec/Gemfile
5 participants