Proposed refactor on OrderedOptions#method_missing

rubyonrails-core
,
1

Hi all :wave:

I’ve been reading through the Rails code over the holidays and noticed an opportunity for a possible reactor that could improve performance in some cases.

I’ve pushed my changes out to Github (on my own fork) but—as directed in the Contribution Guidelines—I’m opening the topic for discussion here first.

Please feel free to review the changes I’ve made in this commit:

My thinking was that getting values from an OrderedOptions instance is probably more common than either setting a value or getting with a bang. I think this is particularly true while code is being executed at runtime, since things like configuration values tend not to change once the app has booted.

I tried a few permutations to optimise for this case, and benchmarked them using Benchmark/IPS.

The commit I’ve linked above was the best performer I could come up with today.

Here are some benchmark results from my own machine. In all cases, only the method_missing method was changed. I’ve omitted the rest of the methods from the examples for brevity.

# Current implementation
class VariantA < Hash
  # ...
  def method_missing(name, *args)
    name_string = +name.to_s
    if name_string.chomp!("=")
      self[name_string] = args.first
    else
      bangs = name_string.chomp!("!")

      if bangs
        self[name_string].presence || raise(KeyError.new(":#{name_string} is blank"))
      else
        self[name_string]
      end
    end
  end
  # ...
end

# Fastest
class VariantB < Hash
  # ...
  def method_missing(name, *args)
    return self[name] unless name.end_with?("=") || name.end_with?("!")

    name_string = name.to_s
    if name_string.slice!(-1) == "="
      return self[name_string] = args.first
    end

    self[name_string].presence || raise(KeyError.new(":#{name_string} is blank"))
  end
  #  ...
end

class VariantC < Hash
  #  ...
  def method_missing(name, *args)
    return self[name] unless name.end_with?("=") || name.end_with?("!")

    name_string = name.to_s
    name_string.slice!(-1)
    if name.end_with?("=")
      return self[name_string] = args.first
    end

    self[name_string].presence || raise(KeyError.new(":#{name_string} is blank"))
  end
  # ...
end


test_lambda = lambda do |test_class|
  h = test_class.new
  # Extracted from example in docs
  h.boy = 'John'
  h.girl = 'Mary'
  h.boy
  h.girl
  h.dog
end

Benchmark.ips do |test|
  test.iterations = 1

  test.report("Variant A") do
    test_lambda.call(VariantA)
  end
  test.report("Variant B") do
    test_lambda.call(VariantB)
  end
  test.report("Variant C") do
    test_lambda.call(VariantC)
  end
  test.compare!
end

Results

The benchmark results on my machine look like this:

Warming up --------------------------------------
           Variant A    16.082k i/100ms
           Variant B    20.174k i/100ms
           Variant C    19.369k i/100ms
Calculating -------------------------------------
           Variant A    164.454k (± 2.9%) i/s -    836.264k in   5.089449s
           Variant B    201.225k (± 1.7%) i/s -      1.009M in   5.014209s
           Variant C    192.310k (± 2.0%) i/s -    968.450k in   5.037718s

Comparison:
           Variant B:   201225.1 i/s
           Variant C:   192310.2 i/s - 1.05x  (± 0.00) slower
           Variant A:   164453.9 i/s - 1.22x  (± 0.00) slower

I would welcome feedback on this, and whether or not the community would welcome a PR with these changes.

Thanks, and happy New Year!

1 Like