-
-
Notifications
You must be signed in to change notification settings - Fork 233
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
Conversation
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' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
2685136
to
5c018e5
Compare
@Fryguy As you say, there is no need to replace ostruct. I created PoC to compare ostruct with method_missing.
PoC & BenchmarkPoC & 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:
|
@taketo1113 updating changelog would be useful |
@pkuczynski Currently, CHANGELOG does not have a section of Unreleased. |
Yes please... |
5c018e5
to
2a36077
Compare
@pkuczynski I updated CHANGELOG. |
This issue can be closed once merged 👍 |
@Fryguy can we get a release for this fix? 😄 |
I'd love to but I'm not a gem owner. @pkuczynski can do it once this is merged |
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 😋 |
Sure! Done. |
It fixes a warning of loading ostruct gem from standard library with Ruby 3.3.5 and 3.4+.
The warning message is following:
Related Links
Additional information
This warning category is
performance
.It may be better to reduce the dependency of ostruct.
ruby/ostruct#56
Closes #365