At Discourse we still have not upgraded to Ruby 2.7 in production. I think it is quite a shame, and the biggest driver for not upgrading was that the benefits of Ruby 2.7 were small compared to dealing with various loud deprecations. We did not notice that Discourse runs any faster on 2.7 maybe it is all the deprecation accounting, I donāt know.
Recently, we upgraded to latest Rails to reduce warnings, but we are left with warnings from gems and warnings from Discourse core.
Examples of Discourse warnings are:
/home/sam/Source/discourse/lib/freedom_patches/inflector_backport.rb:30: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/home/sam/.gem/ruby/2.7.1/gems/activesupport-6.0.3/lib/active_support/inflector/methods.rb:128: warning: The called method `humanize_without_cache' is defined here
/home/sam/Source/discourse/lib/plugin/instance.rb:230: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/home/sam/.gem/ruby/2.7.1/gems/activemodel-6.0.3/lib/active_model/callbacks.rb:130: warning: The called method is defined here
/home/sam/Source/discourse/lib/plugin/instance.rb:230: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/home/sam/.gem/ruby/2.7.1/gems/activemodel-6.0.3/lib/active_model/callbacks.rb:144: warning: The called method is defined here
/home/sam/Source/discourse/app/models/user.rb:212: warning: deprecated Object#=~ is called on Array; it always returns nil
/home/sam/Source/discourse/app/models/user.rb:212: warning: deprecated Object#=~ is called on Array; it always returns nil
Examples of gem warnings are:
/home/sam/.gem/ruby/2.7.1/gems/nokogumbo-2.0.2/lib/nokogumbo/html5.rb:22: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/home/sam/.gem/ruby/2.7.1/gems/nokogumbo-2.0.2/lib/nokogumbo/html5/document.rb:4: warning: The called method `parse' is defined here
/home/sam/.gem/ruby/2.7.1/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on TrueClass; it always returns nil
/home/sam/.gem/ruby/2.7.1/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on TrueClass; it always returns nil
..../home/sam/.gem/ruby/2.7.1/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on TrueClass; it always returns nil
../home/sam/.gem/ruby/2.7.1/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on TrueClass; it always returns nil
........./home/sam/.gem/ruby/2.7.1/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on TrueClass; it always returns nil
Dealing with our warnings is feasible albeit annoying. More on this soon. Dealing with gem warnings can be a nightmare.
For example to deal with the sassc we are going to have to start with upgrading sassc we saw recent versions of this gem segfault so we have held back for a bit, getting us to latest is probably us spending weeks of debugging. Further more, gem authors take time to approve PRs and push new versions out there (understandably) so this is going to take a while.
Just silencing deprecations feels wrong to us as a path forward. If Ruby, by default, is being loud I feel we just have to deal with it.
Dealing with the Discourse side of things now comes with some fundamental problems. Firstly I am uneasy with the whole ruby2_keywords thing, it feels like I am adding technical debt to my code when I hack it in. This is all code that I am going to have to remove / re-implement.
Half of our āwe did it to ourselvesā warnings are coming from our Inflector memoizer. https://github.com/discourse/discourse/blob/4601833e4ed98aaaaf90c0ddab2bfebfae42db7c/lib/freedom_patches/inflector_backport.rb#L8-L40. The Rails inflector proved to be a bit of a hot spot for us (and Shopify also confirm that some methods are for them) so we introduced an LRU cache in front to memoize methods.
In Ruby 2.6, a naive implementation is very easy to reason about, it goes something like this:
def memoize_26(method_name)
cache = {}
uncached = "#{method_name}_without_cache"
alias_method uncached, method_name
define_method(method_name) do |*arguments|
found = true
data = cache.fetch(arguments) { found = false }
unless found
cache[arguments] = data = public_send(uncached, *arguments)
end
data
end
end
It can be done a bit faster, but the code is easy to reason about and it has worked for years.
Now Ruby 2.7 comes along and I need to call ruby2_keywords on this dynamic method (and check if we can do this) and carry tech debt going forward, or implement it differently.
... is not going to work for me, cause I need the values passed in so I can do my cache lookup.
Simply adding **kwargs unconditionally to this memoizer means that now my memoizer just got slower for methods with no keyword args, I need to construct a new array for the cache lookup with both kwargs and args
So if I want something that is usually as fast as it used to be I am stuck with this:
def memoize_27_v2(method_name)
uncached = "#{method_name}_without_cache"
alias_method uncached, method_name
cache = "MEMOIZE_#{method_name}"
params = instance_method(method_name).parameters
has_kwargs = params.any? {|t, name| "#{t}".start_with? "key"}
has_args = params.any? {|t, name| !"#{t}".start_with? "key"}
args = []
args << "args" if has_args
args << "kwargs" if has_kwargs
args_text = args.map do |n|
n == "args" ? "*args" : "**kwargs"
end.join(",")
class_eval <<~RUBY
#{cache} = {}
def #{method_name}(#{args_text})
found = true
all_args = #{args.length === 2 ? "[args, kwargs]" : args[0]}
data = #{cache}.fetch(all_args) { found = false }
unless found
#{cache}[all_args] = data = public_send(:#{uncached} #{args.empty? ? "" : ", #{args_text}"})
end
data
end
RUBY
end
Even with all this wizardry I can not recover the performance of the old *args capture cause I am stuck constructing a new array for the cache lookup if args and kwargs are there at the same time.
My old method which was easier to reason about is 1.3 times faster cause I am not stuck generating an array for the lookup.
Full benchmark here:
What do I think should be done
Huge +1 on
- Allow for some new syntax that allows a global capture of both kwargs and args (
...a is not going to work cause it will clash):
def foo(bar)
end
def x(***a)
p a
foo ***a
end
# maybe
def x(...)
a = ...
p a
foo ...
end
-
Remove all kwarg deprecation accounting and warnings from Ruby 2.7
-
The train has probably left the station but ruby2_keywordsā¦ I wish it could be removed, if we had support for ***a in 3 we could just eval ourselves out of the syntax clash in a switch for the places where we need cross Ruby support. I donāt know.
-
Consider, why did we allow so many deprecations to go silent for years and suddenly we are very loud?
irb(main):001:0> URI.escape("a")
=> "a"
irb(main):002:0> RUBY_VERSION
=> "2.6.5"
# vs
irb(main):001:0> URI.escape("a")
(irb):1: warning: URI.escape is obsolete
=> "a"
irb(main):002:0> RUBY_VERSION
=> "2.7.1"
This for example has been deprecated for almost 15 years already, but just now got loud.
I extremely appreciate everyone in the MRI core team and I understand the enromity of this conundrum, but sadly the loud deprecation notices are holding people up from upgrading Ruby which is achieving the opposite of one of the core missions of the MRI team which is to keep everyone upgraded.
Discourse / Basecamp / Shopify / GitHub not running 2.7 yet at scale means that tons of real world performance testing is not happening and all sorts of bugs will take a lot longer to find.
@eileencodes has been working on upgrading GitHub recently, perhaps she has more insight.
I know @rafaelfranca has also been very vocal about the delegation change, perhaps he has some stuff to add here.
