From a5aff14976b1b08bfdc795f1246f8b70513b2456 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Joubert?= Date: Mon, 26 Oct 2026 10:14:05 +0300 Subject: [PATCH 1/3] Parse and serialize declarations with less than half the allocations Value#value= copied its string through sub even with nothing to clean; it now copies only when a trailing semicolon, !important or surrounding whitespace has to go, and freezes the result with -value. reduce and each_with_object over the declarations hash built a [key, value] pair per entry, casecmp? case-folded both strings, replace_declaration! rebuilt its hash through zip, and merge stripped already stripped values. A parsed declaration goes from 10.6 to 4.6 objects, declarations_to_s from 21 to 7; inlining our 336 emails with premailer allocates 32% fewer objects, with byte-for-byte identical output. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + lib/css_parser.rb | 7 ++-- lib/css_parser/rule_set.rb | 42 ++++++++++++------- .../rule_set/test_declarations_allocations.rb | 35 ++++++++++++++++ test/test_helper.rb | 16 +++++++ 5 files changed, 82 insertions(+), 19 deletions(-) create mode 100644 test/rule_set/test_declarations_allocations.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 52e1cbe..e93c128 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ### Unreleased * Speed up `RuleSet::Declarations#to_s` ~20x: its clean-up regexp matched the empty string at every character (`[\n\r\f\t]*`), making `gsub!` replace at every position; the unreachable `\s+$` branch is dropped, output unchanged * Allocate less in `RuleSet`: skip the downcased copy of already normalized property names, and build dimension shorthands without enumerators +* 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` diff --git a/lib/css_parser.rb b/lib/css_parser.rb index 62e87da..c39a603 100644 --- a/lib/css_parser.rb +++ b/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! diff --git a/lib/css_parser/rule_set.rb b/lib/css_parser/rule_set.rb index cff4bd7..cd6c6cf 100644 --- a/lib/css_parser/rule_set.rb +++ b/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,14 @@ 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! + value = value.to_s + value = value.sub(TRAILING_SEMICOLON_RX, '') if value.include?(SEMICOLON) + self.important = value.include?('!') && 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 = -value end def to_s @@ -70,7 +77,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 +107,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 +171,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 +196,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 +207,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_index { |index| declarations[replacement_keys[index]] = 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! @@ -675,12 +685,12 @@ 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) + next if property.empty? || value.empty? || value.casecmp(IMPORTANT) == 0 - add_declaration!(property, value) + add_declaration!(property, value.freeze) continuation = nil end end diff --git a/test/rule_set/test_declarations_allocations.rb b/test/rule_set/test_declarations_allocations.rb new file mode 100644 index 0000000..ff2899d --- /dev/null +++ b/test/rule_set/test_declarations_allocations.rb @@ -0,0 +1,35 @@ +# 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 + value = +' 1px !important; ' + + RuleSet::Declarations::Value.new(value) + + assert_equal [' 1px !important; ', false], [value, value.frozen?] + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index 6411176..9f38b97 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -7,3 +7,19 @@ require 'net/http' require 'webrick' require 'css_parser' + +module AllocationCounting + def allocations + skip 'GC.stat(:total_allocated_objects) is MRI only' unless RUBY_ENGINE == 'ruby' + + yield + GC.disable + Array.new(2) do + before = GC.stat(:total_allocated_objects) + yield + GC.stat(:total_allocated_objects) - before + end.min + ensure + GC.enable + end +end From 03c70d2e8dee213c89d5e3bfe40d7e77a349992c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Joubert?= Date: Mon, 26 Oct 2026 12:49:44 +0300 Subject: [PATCH 2/3] Cover clean unfrozen values in the allocation tests, take the minimum of 3 runs The unfrozen-value test only used a value that needed cleaning, so sub copied it before -value ran; a clean one now goes straight through -value and must come out neither frozen nor mutated. GC.stat counts allocations of every thread, so the helper keeps the minimum of three runs. Co-Authored-By: Claude Opus 5.5 --- test/rule_set/test_declarations_allocations.rb | 8 +++++--- test/test_helper.rb | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/test/rule_set/test_declarations_allocations.rb b/test/rule_set/test_declarations_allocations.rb index ff2899d..176a50f 100644 --- a/test/rule_set/test_declarations_allocations.rb +++ b/test/rule_set/test_declarations_allocations.rb @@ -26,10 +26,12 @@ def test_frozen_values_are_kept_without_copy end def test_unfrozen_values_are_neither_frozen_nor_mutated - value = +' 1px !important; ' + clean = +'1px' + padded = +' 1px !important; ' - RuleSet::Declarations::Value.new(value) + RuleSet::Declarations::Value.new(clean) + RuleSet::Declarations::Value.new(padded) - assert_equal [' 1px !important; ', false], [value, value.frozen?] + assert_equal [['1px', false], [' 1px !important; ', false]], [[clean, clean.frozen?], [padded, padded.frozen?]] end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 9f38b97..c39b380 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -14,7 +14,7 @@ def allocations yield GC.disable - Array.new(2) do + Array.new(3) do before = GC.stat(:total_allocated_objects) yield GC.stat(:total_allocated_objects) - before From 6335c03e14bb8fc870e768751a161a2cbb93e7ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Joubert?= Date: Tue, 27 Oct 2026 12:35:28 +0300 Subject: [PATCH 3/3] Drop the include? pre-checks, keep casecmp as a predicate, explain why Value#value= now guards sub and strip with the regexps themselves: same allocations, the include? short-cuts only made value= alone 15-19% faster, which does not show on a whole parse. Comments say why the guards and -value are there, and why casecmp? is avoided (it case-folds both strings: two objects per declaration). The rebuilt declarations hash reads with each_with_index. Co-Authored-By: Claude Opus 5.5 --- lib/css_parser/rule_set.rb | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/css_parser/rule_set.rb b/lib/css_parser/rule_set.rb index ae3696d..6fa6c16 100644 --- a/lib/css_parser/rule_set.rb +++ b/lib/css_parser/rule_set.rb @@ -52,13 +52,16 @@ def initialize(value, important: nil) end def value=(value) + # 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.include?(SEMICOLON) - self.important = value.include?('!') && value.match?(CssParser::IMPORTANT_IN_PROPERTY_RX) + 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 may still be the caller's string: -value freezes a copy instead of it @value = -value end @@ -208,7 +211,7 @@ def replace_declaration!(property, replacements, preserve_importance: false) replacement_values.insert(property_index, *replacements.values) self.declarations = {} - replacement_keys.each_index { |index| declarations[replacement_keys[index]] = replacement_values[index] } + replacement_keys.each_with_index { |key, index| declarations[key] = replacement_values[index] } end def to_s(options = {}) @@ -680,7 +683,8 @@ def parse_declarations!(block) # :nodoc: value = decs[colon + 1, decs.length] property.strip! value.strip! - next if property.empty? || value.empty? || value.casecmp(IMPORTANT) == 0 + # 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.freeze) continuation = nil