diff --git a/CHANGELOG.md b/CHANGELOG.md index 57632be..2fa542f 100644 --- a/CHANGELOG.md +++ b/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` diff --git a/lib/css_parser.rb b/lib/css_parser.rb index 97f4f47..f175b63 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 375f871..6fa6c16 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,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 diff --git a/test/rule_set/test_declarations_allocations.rb b/test/rule_set/test_declarations_allocations.rb new file mode 100644 index 0000000..176a50f --- /dev/null +++ b/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