Parse declarations with 2.3x fewer allocations (−32% per email through premailer)

CHANGELOG.md

@@ -5,6 +5,7 @@
 * Allocate less in `RuleSet`: skip the downcased copy of already normalized property names, and build dimension shorthands without enumerators
 * Allocate less when expanding and creating dimension shorthands, and skip the colour regexp on values without a comma
 * Return already sanitized media types as-is from `sanitize_media_query`, and allocate less when splitting selectors and counting ids
+* Allocate less than half the objects when parsing and serializing declarations: values are copied only when they need cleaning, and hashes are iterated without building `[key, value]` pairs
 
 ### Version 3.3.0
 * Fix Arbitrary local-file read via attacker-controlled `@import`

lib/css_parser.rb

@@ -85,9 +85,10 @@ def self.merge(*rule_sets)
       end
     end
 
-    merged = properties.each_with_object(RuleSet.new(nil, nil)) do |(property, details), rule_set|
-      value = details[:value].strip
-      rule_set[property.strip] = details[:is_important] ? "#{value.gsub(/;\Z/, '')}!important" : value
+    merged = RuleSet.new(nil, nil)
+    properties.each do |property, details|
+      value = details[:value]
+      merged[property] = details[:is_important] ? "#{value.gsub(/;\Z/, '')}!important" : value
     end
 
     merged.create_shorthand!

lib/css_parser/rule_set.rb

@@ -19,6 +19,9 @@ class RuleSet
     NUMBER_OF_DIMENSIONS = 4
 
     NORMALIZED_PROPERTY_RX = /\A[-a-z0-9_]+\z/.freeze
+    BLANK_VALUE_RX = /\A[\s\0]*\z/.freeze
+    SURROUNDING_WHITESPACE_RX = /\A[\s\0]|[\s\0]\z/.freeze
+    TRAILING_SEMICOLON_RX = /\s*;\s*\Z/.freeze
 
     DIMENSIONS = [
       ['margin', %w[margin-top margin-right margin-bottom margin-left]],
@@ -37,6 +40,8 @@ class RuleSet
     RPAREN = ')'.freeze
     IMPORTANT = '!important'.freeze
     class Declarations
+      EMPTY_DECLARATIONS = {}.freeze
+
       class Value
         attr_reader :value
         attr_accessor :important
@@ -47,12 +52,17 @@ def initialize(value, important: nil)
         end
 
         def value=(value)
-          value = value.to_s.sub(/\s*;\s*\Z/, '')
-          self.important = !value.slice!(CssParser::IMPORTANT_IN_PROPERTY_RX).nil?
-          value.strip!
+          # sub and strip always return a copy, and the caller's string must not be mutated:
+          # only copy when there is something to remove, so a clean value allocates nothing
+          value = value.to_s
+          value = value.sub(TRAILING_SEMICOLON_RX, '') if value.match?(TRAILING_SEMICOLON_RX)
+          self.important = value.match?(CssParser::IMPORTANT_IN_PROPERTY_RX)
+          value = value.sub(CssParser::IMPORTANT_IN_PROPERTY_RX, '') if important
+          value = value.strip if value.match?(SURROUNDING_WHITESPACE_RX)
           raise ArgumentError, 'value is empty' if value.empty?
 
-          @value = value.freeze
+          # value may still be the caller's string: -value freezes a copy instead of it
+          @value = -value
         end
 
         def to_s
@@ -70,7 +80,7 @@ def ==(other)
 
       def_delegators :declarations, :each, :each_value
 
-      def initialize(declarations = {})
+      def initialize(declarations = EMPTY_DECLARATIONS)
         self.declarations = {}
         declarations.each { |property, value| add_declaration!(property, value) }
       end
@@ -100,7 +110,7 @@ def []=(property, value)
 
         if value.is_a?(Value) && (!currently_important || value.important)
           declarations[property] = value
-        elsif value.to_s.strip.empty?
+        elsif value.to_s.match?(BLANK_VALUE_RX)
           delete property
         else
           value = Value.new(value)
@@ -164,18 +174,19 @@ def replace_declaration!(property, replacements, preserve_importance: false)
 
         # We should preserve subsequent declarations of the same properties
         # and prior important ones if replacement one is not important
-        replacements = replacement_declarations.each.with_object({}) do |(key, replacement), result|
+        replacements = {}
+        replacement_declarations.declarations.each do |key, replacement|
           existing = declarations[key]
 
           # No existing -> set
           unless existing
-            result[key] = replacement
+            replacements[key] = replacement
             next
           end
 
           # Replacement more important than existing -> replace
           if replacement.important && !existing.important
-            result[key] = replacement
+            replacements[key] = replacement
             replaced_index = replacement_keys.index(key)
             replacement_keys.delete_at(replaced_index)
             replacement_values.delete_at(replaced_index)
@@ -188,7 +199,7 @@ def replace_declaration!(property, replacements, preserve_importance: false)
 
           # Existing and replacement importance are the same,
           # value which is declared later wins
-          result[key] = replacement if property_index > replacement_keys.index(key)
+          replacements[key] = replacement if property_index > replacement_keys.index(key)
         end
 
         return if replacements.empty?
@@ -199,13 +210,15 @@ def replace_declaration!(property, replacements, preserve_importance: false)
         replacement_values.delete_at(property_index)
         replacement_values.insert(property_index, *replacements.values)
 
-        self.declarations = replacement_keys.zip(replacement_values).to_h
+        self.declarations = {}
+        replacement_keys.each_with_index { |key, index| declarations[key] = replacement_values[index] }
       end
 
       def to_s(options = {})
-        str = declarations.reduce(+'') do |memo, (prop, value)|
+        str = +''
+        declarations.each do |prop, value|
           importance = options[:force_important] || value.important ? ' !important' : ''
-          memo << "#{prop}: #{value.value}#{importance}; "
+          str << prop << ': ' << value.value << importance << '; '
         end
         str.gsub!(/^[\s^({)]+|[\n\r\f\t]+/, '')
         str.strip!
@@ -667,12 +680,13 @@ def parse_declarations!(block) # :nodoc:
         next unless (colon = decs.index(COLON))
 
         property = decs[0, colon]
-        value = decs[(colon + 1)..]
+        value = decs[colon + 1, decs.length]
         property.strip!
         value.strip!
-        next if property.empty? || value.empty? || value.casecmp?(IMPORTANT)
+        # casecmp? case-folds both strings, allocating two of them per declaration
+        next if property.empty? || value.empty? || value.casecmp(IMPORTANT).zero?
 
-        add_declaration!(property, value)
+        add_declaration!(property, value.freeze)
         continuation = nil
       end
     end

test/rule_set/test_declarations_allocations.rb

@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+require_relative '../test_helper'
+
+class RuleSetDeclarationsAllocationsTest < Minitest::Test
+  include CssParser
+  include AllocationCounting
+
+  PADDINGS = Array.new(20) { |i| "padding-#{%w[top right bottom left][i % 4]}: #{i}px" }.join('; ')
+  BLOCK = "#{PADDINGS}; color: red !important".freeze
+
+  def test_parsing_declarations_allocates_less_than_six_objects_per_declaration
+    assert_operator allocations { RuleSet.new(block: BLOCK) }, :<, 6 * 21
+  end
+
+  def test_serializing_declarations_does_not_allocate_per_declaration
+    rule_set = RuleSet.new(block: BLOCK)
+
+    assert_operator allocations { rule_set.declarations_to_s }, :<, 10
+  end
+
+  def test_frozen_values_are_kept_without_copy
+    value = '1px'
+
+    assert_same value, RuleSet::Declarations::Value.new(value).value
+  end
+
+  def test_unfrozen_values_are_neither_frozen_nor_mutated
+    clean = +'1px'
+    padded = +' 1px !important; '
+
+    RuleSet::Declarations::Value.new(clean)
+    RuleSet::Declarations::Value.new(padded)
+
+    assert_equal [['1px', false], [' 1px !important; ', false]], [[clean, clean.frozen?], [padded, padded.frozen?]]
+  end
+end