Cache header normalization to reduce object allocation

[alexcwatt]

I'm using this gem to make a lot of requests in a multi-threaded environment. I've noticed that header normalization contributes to a large percentage of the overall objects allocated.

I expect that most services will come across some of the same headers again and again. In my case, I haven't counted the different headers that are encountered in production, but I only issue traffic to the same service so I am confident that I'm parsing the same headers many times.

This PR introduces a cache for header normalization to reduce object allocation. Here's a benchmark script to validate the improvement (you can put this in the root of the repository as bench.rb, then run with bundle exec ruby bench.rb for example):

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem "benchmark-ips"
  gem "benchmark-memory"
  gem "http", path: "."
end

require "http"

require "benchmark/ips"
require "benchmark/memory"

puts "== Benchmark speed"
Benchmark.ips do |x|
  x.report("HTTP::Headers#add") {
    headers = HTTP::Headers.new
    headers.add("content_type", "application/json")
    headers.add("some-other-header", "some-value")
    headers.add("another-header", "another-value")
    headers.add("yet-another-header", "yet-another-value")
  }
end

puts "\n== Benchmark memory"
Benchmark.memory do |x|
  x.report("HTTP::Headers#add same header") {
    headers = HTTP::Headers.new
    headers.add("content_type", "application/json")
    headers.add("content_type", "application/json")
    headers.add("content_type", "application/json")
    headers.add("content_type", "application/json")
  }

  x.report("HTTP::Headers#add different header") {
    headers = HTTP::Headers.new
    headers.add("content_type", "application/json")
    headers.add("some-other-header", "some-value")
    headers.add("another-header", "another-value")
    headers.add("yet-another-header", "yet-another-value")
  }
end

Results on main

== Benchmark speed
ruby 3.2.1 (2023-02-08 revision 31819e82c8) [x86_64-darwin22]
Warming up --------------------------------------
   HTTP::Headers#add     7.672k i/100ms
Calculating -------------------------------------
   HTTP::Headers#add     76.731k (± 6.1%) i/s -    383.600k in   5.024653s

== Benchmark memory
Calculating -------------------------------------
HTTP::Headers#add same header
                         2.832k memsize (     0.000  retained)
                        50.000  objects (     0.000  retained)
                         5.000  strings (     0.000  retained)
HTTP::Headers#add different header
                         3.648k memsize (    80.000  retained)
                        58.000  objects (     1.000  retained)
                        19.000  strings (     1.000  retained)

Results on this branch

== Benchmark speed
ruby 3.2.1 (2023-02-08 revision 31819e82c8) [x86_64-darwin22]
Warming up --------------------------------------
   HTTP::Headers#add    30.330k i/100ms
Calculating -------------------------------------
   HTTP::Headers#add    299.935k (± 2.7%) i/s -      1.516M in   5.059825s

== Benchmark memory
Calculating -------------------------------------
HTTP::Headers#add same header
                         1.040k memsize (     0.000  retained)
                        18.000  objects (     0.000  retained)
                         2.000  strings (     0.000  retained)
HTTP::Headers#add different header
                         1.040k memsize (    80.000  retained)
                        18.000  objects (     1.000  retained)
                         8.000  strings (     1.000  retained)

Analysis

I wanted to use Benchmark.ips to make sure what I'm adding doesn't regress performance. This PR improves performance on the benchmark because a single HeaderNormalizer instance is memoized in the Header class and reused for all the benchmarks, so once we've normalized a header once, there is no additional work the next time.

However, what I really care about is the object allocations. We can see that this PR increases the retained memory, as expected, since we are now persisting header normalized values in a cache. But the win here is the reduction in objects allocated.

Follow up

I just wanted to get this out for your feedback! Please let me know what you think and whether you're open to something like this in the gem. If not, perhaps we can explore a pattern that would make it easier for me to extend the gem...

[ixti]

If we're going to use cache, we should ensure that the normalized names in the cache are immutable:

name = -name.to_s
@cache[name] ||= -normalize_header(name)

Otherwise it might cause unpleasant surprise, when user uses something like:

response.headers.to_h.keys.each(&:upcase!)

[ixti]

After spectacular failure with LRU cache implementation (thread safe variant I could think of right now is way too slow), WDYT about this patch:

• Improve class structure consistency a bit (to follow ruby style guide)
• Remove unused methods from Cache
• Ensure cache keys are not modified
• Ensure cached values are not modified
• Truncate cache store before adding a new element

diff --git a/lib/http/headers.rb b/lib/http/headers.rb
index 89ff78b..5f28561 100644
--- a/lib/http/headers.rb
+++ b/lib/http/headers.rb
@@ -13,6 +13,33 @@ module HTTP
     extend Forwardable
     include Enumerable
 
+    class << self
+      # Coerces given `object` into Headers.
+      #
+      # @raise [Error] if object can't be coerced
+      # @param [#to_hash, #to_h, #to_a] object
+      # @return [Headers]
+      def coerce(object)
+        unless object.is_a? self
+          object = case
+                   when object.respond_to?(:to_hash) then object.to_hash
+                   when object.respond_to?(:to_h)    then object.to_h
+                   when object.respond_to?(:to_a)    then object.to_a
+                   else raise Error, "Can't coerce #{object.inspect} to Headers"
+                   end
+        end
+
+        headers = new
+        object.each { |k, v| headers.add k, v }
+        headers
+      end
+      alias [] coerce
+
+      def normalizer
+        @normalizer ||= Headers::Normalizer.new
+      end
+    end
+
     # Class constructor.
     def initialize
       # The @pile stores each header value using a three element array:
@@ -188,40 +215,11 @@ module HTTP
       dup.tap { |dupped| dupped.merge! other }
     end
 
-    class << self
-      # Coerces given `object` into Headers.
-      #
-      # @raise [Error] if object can't be coerced
-      # @param [#to_hash, #to_h, #to_a] object
-      # @return [Headers]
-      def coerce(object)
-        unless object.is_a? self
-          object = case
-                   when object.respond_to?(:to_hash) then object.to_hash
-                   when object.respond_to?(:to_h)    then object.to_h
-                   when object.respond_to?(:to_a)    then object.to_a
-                   else raise Error, "Can't coerce #{object.inspect} to Headers"
-                   end
-        end
-
-        headers = new
-        object.each { |k, v| headers.add k, v }
-        headers
-      end
-      alias [] coerce
-    end
-
     private
 
-    class << self
-      def header_normalizer
-        @header_normalizer ||= Headers::Normalizer.new
-      end
-    end
-
     # Transforms `name` to canonical HTTP header capitalization
     def normalize_header(name)
-      self.class.header_normalizer.normalize(name)
+      self.class.normalizer.call(name)
     end
 
     # Ensures there is no new line character in the header value
diff --git a/lib/http/headers/normalizer.rb b/lib/http/headers/normalizer.rb
index 7c260d0..e3eb3de 100644
--- a/lib/http/headers/normalizer.rb
+++ b/lib/http/headers/normalizer.rb
@@ -10,15 +10,41 @@ module HTTP
       # @see http://tools.ietf.org/html/rfc7230#section-3.2
       COMPLIANT_NAME_RE = /\A[A-Za-z0-9!#$%&'*+\-.^_`|~]+\z/
 
-      MAX_CACHE_SIZE = 200
+      NAME_PARTS_SEPARATOR_RE = /[\-_]/
+
+      # @private
+      # Normalized header names cache
+      class Cache
+        MAX_SIZE = 200
+
+        def initialize
+          @store = {}
+        end
+
+        def get(key)
+          @store[key]
+        end
+        alias [] get
+
+        def set(key, value)
+          # Maintain cache size
+          @store.delete(@store.each_key.first) while MAX_SIZE <= @store.size
+
+          @store[key] = value
+        end
+        alias []= set
+      end
 
       def initialize
-        @cache = Cache.new(MAX_CACHE_SIZE)
+        @cache = Cache.new
       end
 
       # Transforms `name` to canonical HTTP header capitalization
-      def normalize(name)
-        @cache[name] ||= normalize_header(name)
+      def call(name)
+        name  = -name.to_s
+        value = (@cache[name] ||= -normalize_header(name))
+
+        value.dup
       end
 
       private
@@ -32,51 +58,11 @@ module HTTP
       def normalize_header(name)
         return name if CANONICAL_NAME_RE.match?(name)
 
-        normalized = name.split(/[\-_]/).each(&:capitalize!).join("-")
-
+        normalized = name.split(NAME_PARTS_SEPARATOR_RE).each(&:capitalize!).join("-")
         return normalized if COMPLIANT_NAME_RE.match?(normalized)
 
         raise HeaderError, "Invalid HTTP header field name: #{name.inspect}"
       end
-
-      class Cache
-        def initialize(max_size)
-          @max_size = max_size
-          @cache = {}
-        end
-
-        def get(key)
-          @cache[key]
-        end
-
-        def set(key, value)
-          @cache[key] = value
-
-          # Maintain cache size
-          return unless @cache.size > @max_size
-
-          oldest_key = @cache.keys.first
-          @cache.delete(oldest_key)
-        end
-
-        def size
-          @cache.size
-        end
-
-        def key?(key)
-          @cache.key?(key)
-        end
-
-        def [](key)
-          get(key)
-        end
-
-        def []=(key, value)
-          set(key, value)
-        end
-      end
-
-      private_constant :Cache
     end
   end
 end

PS: I think for specs we should add rspec-memory to test objects allocations instead of cache size

---

UPDATE: Updated proposed patch

My LRU proposal was bad

[alexcwatt]

@ixti Thank you for the review and ideas! I took your patch and updated the tests, including:

• Added a test to ensure that each #call with the same value returns a different object
• Added a test to ensure that #call results aren't frozen
• Added two tests using rspec-memory
• I still like the idea of a test that explicitly checks what happens when the cache is full, so I have some version of that still

Please let me know what you think

[ixti]

Awesome! Thank you!

[tarcieri]

@ixti might be good to cut a release. There are a few merged PRs that haven't been released yet

[alexcwatt]

Thanks for the help here @ixti and the really fast reviews and feedback!

A release would be awesome - I'd love to try this in production.