Cache Gem version checks with a variable #24
Conversation
… values to avoid lots of unnecessary string comparisons
5 similar comments
|
FYI -- def method_returning_boolean
puts "called method"
false
end
[2] pry(main)> @result ||= method_returning_boolean
called method
=> false
[3] pry(main)> @result ||= method_returning_boolean
called method
=> false
[4] pry(main)> @result ||= method_returning_boolean
called method
=> falseI believe you'll need to write it like so: unless defined?(@mass_assignment_security_enabled)
@mass_assignment_security_enabled = (::ActiveRecord::VERSION::MAJOR < 4 || defined?(::ActiveRecord::MassAssignmentSecurity))
end |
|
@dvandersluis You're right, it's not appropriate for a boolean. My mistake. |
|
I remember reading this sometime back - http://www.rubyinside.com/what-rubys-double-pipe-or-equals-really-does-5488.html |
|
@jcalvert - Thanks for tracking this down! Mutable class/module level variables are generally a source of thread-safety problems and are usually best avoided (even if it's likely innocuous in this case). Mind updating your fix to eagerly evaluate the various fields when the |
4 similar comments
|
@jturkel - Fair point - it would be minor but even where it would be innocuous here, you could still effectively get double call if you're say, forking before the method is called the first time because then the value is undefined in each child process. My thought was to use constants, since these should actually, effectively be constants. And since ActiveRecord should already be loaded (via lib/goldiloader.rb) it should be safe to load preemptively like you suggest. Retrospectively there's no incentive to wait until call time to resolve the value and that eliminates the conditional. |
3 similar comments
|
Thanks for the fix @jcalvert. I've pushed release 0.0.9 with the change. |
Cache gem version checks within a variable;
After running
ruby-prof(/~https://github.com/ruby-prof/ruby-prof) on some code, I discovered there was some appreciable chunk of time in code being spent initializingGem::Versionobjects and then comparing them. That lead me to this code here where I noticed these module methods were being called repeatedly.Primarily this seems to come from entry points such as
goldiloader/lib/goldiloader/active_record_patches.rb
Line 115 in 3a0f094
load_targetis being called on collections to initialize objects, the number ofGem::Versionobjects created and comparisons done scale directly with the number of records returned.Since the ActiveRecord version won't be changing once an application is started, we can simply cache this value in a variable as a simple boolean and return this.