I was following along on the Rails repository, where I looked at the latest commits merged into master. I stumbled on ActionController::Renderer that can render a template anywhere in your Rails app. I noticed something else too - it could be faster.

After a general skim, I dug in deeper at #initialize:

# Accepts a custom Rack environment to render templates in.
# It will be merged with ActionController::Renderer.defaults
def initialize(env = {})
  @env = normalize_keys(defaults).merge normalize_keys(env)
  @env['action_dispatch.routes'] = controller._routes
end

Nothing out of the ordinary except I didn’t know what #normalize_keys meant. I read that:

def normalize_keys(env)
  env.dup.tap do |new_env|
    convert_symbols!(new_env)
    # other steps...
  end
end

def convert_symbols!(env)
  env.keys.each do |key|
    if key.is_a? Symbol
      value = env.delete(key)
      key = key.to_s.upcase
      env[key] = value
    end
  end
end

Then I knew normalize meant formatting symbol keys like HTTP headers, so :http_host would be 'HTTP_HOST'. After staring at these methods I noticed duplicated work. To remove it I had to refactor my way around it.

First I simplified #convert_symbols!. Those value and key variables weren’t adding any clarity. Out they went.

def convert_symbols!(env)
  env.keys.each do |key|
    if key.is_a? Symbol
      env[key.to_s.upcase] = env.delete(key)
    end
  end
end

I had a feeling there was a handy method in Active Support which would bring out the true intent. I knew #convert_symbols! only transformed the env hash keys - it didn’t touch any values. Yet both the env variable and the #delete method was used. Those things were a different level of abstraction.

I went to Rails’ GitHub project page, hit t and searched for hash. This file was the first result and was in Active Support. It seemed perfect.

require 'active_support/core_ext/hash/compact'
require 'active_support/core_ext/hash/conversions'
require 'active_support/core_ext/hash/deep_merge'
require 'active_support/core_ext/hash/except'
require 'active_support/core_ext/hash/indifferent_access'
require 'active_support/core_ext/hash/keys'
require 'active_support/core_ext/hash/reverse_merge'
require 'active_support/core_ext/hash/slice'
require 'active_support/core_ext/hash/transform_values'

Not exactly a home run. But one of the files required was called keys and there I found oasis:

# Destructively convert all keys using the block operations.
# Same as transform_keys but modifies +self+.
def transform_keys!
  return enum_for(:transform_keys!) unless block_given?
  keys.each do |key|
    self[yield(key)] = delete(key)
  end
  self
end

That each loop was remarkably similar to the one in #convert_symbols! - not to mention it’s name matched what #convert_symbols! did. Another refactor later I had this:

# At the top of the file.
require 'active_support/core_ext/hash/keys'

# Later...
def convert_symbols!(env)
  env.transform_keys! do |key|
    key.is_a?(Symbol) ? key.to_s.upcase : key
  end
end

There. Intent matched with implementation. The code was simpler to understand, but the duplicated work was still there. Come to think of it…

def normalize_keys(env)
  env.dup.tap do |new_env|
    convert_symbols!(new_env)
    # other steps...
  end
end

def convert_symbols!(env)
  env.transform_keys! do |key|
    key.is_a?(Symbol) ? key.to_s.upcase : key
  end
end

Now instead of just mucking with symbol keys, #convert_symbols! modified every key. Yeah, I made the code slower.

Though there was light ahead. #convert_symbols! wasn’t the only method to loop over every key. So did #normalize_keys via #dup. To #dup env Ruby copied over the keys and values to the new hash (likely using a loop), only to have #convert_symbols! mutate env when it reinserted every key.

It would be simpler to have keys transformed when duplicating. I needed a method that returned a new hash to unify those steps. Good thing #transform_keys was there and helped rid us of #dupstep:

def normalize_keys(env)
  convert_symbols(env).tap do |new_env|
    # other steps...
  end
end

def convert_symbols(env)
  env.transform_keys do |key|
    key.is_a?(Symbol) ? key.to_s.upcase : key
  end
end

I was down to one loop.

After that I thought about #convert_symbols and how it didn’t communicate well. Sure it converted symbols, but it was formatting keys like HTTP headers. I ditched the clutter and renamed it #http_header_format.

Now I had improved readability. The code also did less work. But how much faster was it? And how would I or anyone else even know? Potentially I could knock myself out on glitters of speed, but without a test I’d wake up dizzy for nothing. Instead I needed a benchmark to slam dunk reality into any lofty reasoning. For this a special meet and greet is needed with two racers: the revised against the original.

Though writing a benchmark ain’t that simple. Sure getting the code samples was a copy and paste away. But you need to know how many times to run the benchmark too. The question is: what number of times is enough for a proof? Shit, I have no idea. I just use the benchmark-ips gem.

That gem turns any needed pondering on it’s head by running the code as many times as possible in a second. That’s even where the name comes from: iterations per second. The beauty of having a benchmark is it’s pasteable proof. Attach it and the results to a Pull Request for others to rerun and they can tell I’m not pulling their leg.

Back in the code I copied the original and the revised into report blocks. Those blocks each comprised a test and would output their ips count. I also used #compare! to do the dirty work of drawing a winner. For the env variable I took a known input normalized in #initialize, the renderers defaults. They were always normalized, so if my change made that faster I would have proof.

# renderer_test.rb
require 'benchmark/ips'
require 'active_support/core_ext/hash/keys'

env = {
  http_host: 'example.org',
  https: false,
  method: 'get',
  script_name: '',
  'rack.input' => ''
}

Benchmark.ips do |x|
  x.report('dup + bang transform') {
    env.dup.tap do |new_env|
      new_env.keys.each do |key|
        if key.is_a?(Symbol)
          new_env[key.to_s.upcase] = new_env.delete(key)
        end
      end
    end
  }

  x.report('transform_keys') {
    env.transform_keys do |key|
      key.is_a?(Symbol) ? key.to_s.upcase : key
    end
  }

  x.compare!
end

I ran the benchmark with ruby renderer_test.rb and watched it go:

Calculating -------------------------------------
dup + bang transform     7.239k i/100ms
      transform_keys     8.800k i/100ms
-------------------------------------------------
dup + bang transform     88.735k (± 3.4%) i/s -    448.818k
      transform_keys    113.369k (± 3.6%) i/s -    572.000k

Comparison:
      transform_keys:   113369.5 i/s
dup + bang transform:    88735.4 i/s - 1.28x slower

Woah, the new way was 28% faster!

That’s when I submitted a Pull Request - It was an intermittent travis failure that broke the ruby-head tests and rerunning the build fixed it. Then it got merged and suddenly Kevin the unicorn wanted to be friends.

The process wasn’t as straightforward when I worked on the code. Originally I saw #dup and #convert_symbols! overlapped in work. I went looking in Active Support and I knew it’s Ruby extensions where kept by class name in core_ext. After nosing about in core_ext/hash I found #transform_keys. Then came the benchmark and lastly I renamed the method.

Now if you want to contribute to Rails follow the cornerstone of what I did here. Look at the commits on master and follow the Pull Request linked in the commit. See if there’s things you can improve. It doesn’t have to be a speed boost, there’s probably documentation that could be improved. Check back every couple days. At worst you will see how other people contribute to Rails.

When you do submit a Pull Request explain your reasoning. Don’t leave an empty description, but state your case. Kindly explain how whatever you’re changing currently works, why it can be improved and how to do it.

Take it a step at a time.