Fix selector splitting inside functional pseudo-classes by geoffyoungs · Pull Request #184 · premailer/css_parser

geoffyoungs · 2026-03-20T13:10:34Z

The parse_selectors! method used String#split(',') which broke selectors containing comma-separated lists inside functional pseudo-classes like :is(rect, circle) or :where(.a, .b).

This replaces the naive split with a parenthesis-depth-aware split_selectors method that only splits on commas at the top level (depth 0), preserving commas inside parenthesised expressions.

Pre-Merge Checklist

CHANGELOG.md updated with short summary

The `parse_selectors!` method used `String#split(',')` which broke selectors containing comma-separated lists inside functional pseudo-classes like `:is(rect, circle)` or `:where(.a, .b)`. This replaces the naive split with a parenthesis-depth-aware `split_selectors` method that only splits on commas at the top level (depth 0), preserving commas inside parenthesised expressions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

grosser

logic makes sense, add the suggestion so it does not tank performance and it should be good to go 🤞

grosser · 2026-03-21T00:14:18Z

lib/css_parser/rule_set.rb

      end
    end

+    def split_selectors(selectors)


should add this to stay fast:

Suggested change

def split_selectors(selectors)

def split_selectors(selectors)

return selectors.split(',') unless selectors.include?("(") # fast for simple cases

Comparison: plain (simple): 4437547.5 i/s hybrid (simple): 3727921.3 i/s - 1.19x slower plain (nested): 3502059.9 i/s - 1.27x slower char (simple): 895368.7 i/s - 4.96x slower char (nested): 512457.1 i/s - 8.66x slower hybrid (nested): 498396.6 i/s - 8.90x slower

... actually how about

def split_selectors(selector) return selector.split(',') unless selector.include?("(") # fast for simple cases selectors = [] start = 0 depth = 0 selector.each_char.with_index do |char, i| case char when '(' then depth += 1 when ')' then depth -= 1 when ',' if depth == 0 selectors << selector[start...i] start = i + 1 end end end selectors << selector[start..] selectors end

that performed the same (within 1%) and looks simpler to me

Good call - I've spent some more time looking at performance and persuaded Claude to try a CSS selector aware scan, which seems to be only marginally less performant than .split(',') on newer rubies, especially with YJIT enabled.

grosser · 2026-03-21T00:14:38Z

lib/css_parser/rule_set.rb

+    # Split selector string on commas, but not commas inside parentheses
+    # (e.g. :is(rect, circle) or :where(.a, .b) should not be split).


comment belongs to split_selectors

grosser · 2026-03-21T00:15:23Z

lib/css_parser/rule_set.rb

    end

+    def split_selectors(selectors)
+      result = []


maybe call this selectors and the arg selector

grosser · 2026-03-21T00:16:20Z

lib/css_parser/rule_set.rb

+          depth -= 1
+          current << char
+        when ','
+          if depth > 0


would flip conditional to if depth == 0 since that is the whole idea of this code

grosser · 2026-03-21T00:16:49Z

lib/css_parser/rule_set.rb

+        when ','
+          if depth > 0
+            current << char
+          else


Suggested change

else

else # this selector is done

Replace each_char + string concatenation with: - Fast path: String#split for selectors without parens - Medium path: byte-scan with action table for short strings - Long path: regex split for long strings with parens 5–40× faster on real-world CSS framework selectors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

geoffyoungs · 2026-03-21T16:12:59Z

I've tried benchmarking this to see what the most performant option is and as far as I can tell (using a selection of selectors from popular CSS frameworks) this is about as good as I can get the ruby.

Here's my benchmarking script:

#!/usr/bin/env ruby
# frozen_string_literal: true

# ─────────────────────────────────────────────────────────────────────
# Benchmark: css_parser split_selectors optimisation
#
# Compares the PR #184 each_char implementation against the optimised
# three-branch strategy, using real selectors from popular CSS
# frameworks.
#
# Usage (from css_parser repo root):
#   ruby bench/split_selectors_bench.rb
#
# Or standalone (no gem dependency needed):
#   ruby split_selectors_bench.rb
# ─────────────────────────────────────────────────────────────────────

require "benchmark"

# ═══════════════════════════════════════════════════════════════════
# 1. PR #184 current: each_char with string concatenation
# ═══════════════════════════════════════════════════════════════════
module Original
  def self.split_selectors(selectors)
    result = []
    current = +""
    depth = 0

    selectors.each_char do |char|
      case char
      when "("
        depth += 1
        current << char
      when ")"
        depth -= 1
        current << char
      when ","
        if depth > 0
          current << char
        else
          result << current
          current = +""
        end
      else
        current << char
      end
    end

    result << current unless current.empty?
    result
  end
end

# ═══════════════════════════════════════════════════════════════════
# 2. Reviewer suggestion: each_char.with_index + include? fast path
# ═══════════════════════════════════════════════════════════════════
module ReviewerSuggestion
  def self.split_selectors(selector)
    return selector.split(",") unless selector.include?("(")

    selectors = []
    start = 0
    depth = 0

    selector.each_char.with_index do |char, i|
      case char
      when "(" then depth += 1
      when ")" then depth -= 1
      when ","
        if depth == 0
          selectors << selector[start...i]
          start = i + 1
        end
      end
    end

    selectors << selector[start..]
    selectors
  end
end

# ═══════════════════════════════════════════════════════════════════
# 3. Optimised: action table + CSS-aware skip + regex for long strings
# ═══════════════════════════════════════════════════════════════════
module Optimised
  COMMA_OUTSIDE_PARENS = /,(?![^()]*\))/

  ACTION = Array.new(256, 0)
  ACTION[0x2C] = 1  # ,
  ACTION[0x28] = 2  # (
  ACTION[0x29] = 3  # )
  ACTION[0x2E] = 4  # .
  ACTION[0x23] = 4  # #
  ACTION[0x3A] = 4  # :
  ACTION.freeze

  def self.split_selectors(selectors)
    len = selectors.bytesize
    return [selectors] if len == 0

    if !selectors.include?("(")
      return selectors.split(",")
    end

    if len < 80
      scan_selectors(selectors, len)
    else
      selectors.split(COMMA_OUTSIDE_PARENS)
    end
  end

  def self.scan_selectors(str, len)
    results = []
    segment_start = 0
    depth = 0
    pos = 0

    while pos < len
      case ACTION[str.getbyte(pos)]
      when 0
      when 1
        if depth == 0
          results << str.byteslice(segment_start, pos - segment_start)
          segment_start = pos + 1
        end
      when 2 then depth += 1
      when 3 then depth -= 1
      when 4 then pos += 1
      end
      pos += 1
    end

    results << str.byteslice(segment_start, len - segment_start)
  end
end

# ═══════════════════════════════════════════════════════════════════
# 4. Speed-of-light ceiling: plain split(",") — INCORRECT
# ═══════════════════════════════════════════════════════════════════
module PlainSplit
  def self.split_selectors(selectors)
    selectors.split(",")
  end
end

# ═══════════════════════════════════════════════════════════════════
# Framework selectors
# ═══════════════════════════════════════════════════════════════════
SELECTORS = {
  # Bootstrap 5
  "bs:headings"     => ["h1, h2, h3, h4, h5, h6", "Bootstrap 5"],
  "bs:buttons"      => [".btn-primary, .btn-secondary, .btn-success, .btn-danger, .btn-warning, .btn-info, .btn-light, .btn-dark", "Bootstrap 5"],
  "bs:grid_cols"    => [".col, .col-1, .col-2, .col-3, .col-4, .col-5, .col-6, .col-7, .col-8, .col-9, .col-10, .col-11, .col-12", "Bootstrap 5"],
  "bs:navbar"       => [".navbar-dark .navbar-nav .nav-link, .navbar-dark .navbar-nav .nav-link.active, .navbar-dark .navbar-nav .show > .nav-link", "Bootstrap 5"],
  "bs:reboot"       => ["body, h1, h2, h3, h4, h5, h6, p, ol, ul", "Bootstrap 5"],
  # Tailwind CSS
  "tw:universal"    => ["*, ::before, ::after", "Tailwind CSS"],
  "tw:media"        => ["img, svg, video, canvas, audio, iframe, embed, object", "Tailwind CSS"],
  "tw:inputs"       => ["button, input, optgroup, select, textarea", "Tailwind CSS"],
  "tw:prose_h"      => ['.prose :where(h1, h2, h3, h4, h5, h6):not(:where([class~="not-prose"], [class~="not-prose"] *))', "Tailwind CSS"],
  "tw:prose_list"   => ['.prose :where(ol, ul):not(:where([class~="not-prose"], [class~="not-prose"] *))', "Tailwind CSS"],
  # normalize.css
  "norm:buttons"    => ['button, [type="button"], [type="reset"], [type="submit"]', "normalize.css"],
  # CSS Resets
  "reset:where_all" => [":where(html, body, h1, h2, h3, h4, h5, h6, p, ol, ul, figure, blockquote, dl, dd)", "CSS Reset"],
  "reset:is_h"      => [":is(h1, h2, h3, h4, h5, h6)", "CSS Reset"],
  # Pico CSS
  "pico:form"       => ['input:not([type="checkbox"], [type="radio"], [type="range"], [type="file"]), select, textarea', "Pico CSS"],
  # Bulma
  "bulma:buttons"   => [".button, .buttons .button:not(:last-child), .buttons .button:not(:first-child)", "Bulma"],
  # Foundation
  "found:grid"      => [".small-1, .small-2, .small-3, .small-4, .small-5, .small-6, .small-7, .small-8, .small-9, .small-10, .small-11, .small-12", "Foundation"],
  # Real-world app
  "app:sidebar"     => [".sidebar .nav-item > .nav-link, .sidebar .nav-item > .nav-link.active, .sidebar .nav-item:hover > .nav-link", "Real app"],
  "app:theme"       => [':root, :root[data-theme="dark"], :root[data-theme="light"]', "Real app"],
}

# ═══════════════════════════════════════════════════════════════════
# Correctness
# ═══════════════════════════════════════════════════════════════════
TESTS = {
  ".a, .b"                            => [".a", " .b"],
  ":is(.a,.b)"                        => [":is(.a,.b)"],
  ".a, :is(.b,.c), .d"               => [".a", " :is(.b,.c)", " .d"],
  ":is(:not(.a,.b),.c), .d"          => [":is(:not(.a,.b),.c)", " .d"],
  ":is(:not(:where(.a,.b),.c),.d), .e" => [":is(:not(:where(.a,.b),.c),.d)", " .e"],
  ".solo"                             => [".solo"],
}

puts "#{RUBY_DESCRIPTION}"
jit = if defined?(RubyVM::YJIT) && RubyVM::YJIT.enabled?
  "YJIT"
elsif defined?(RubyVM::RJIT) && RubyVM::RJIT.enabled?
  "RJIT"
else
  "no JIT"
end
puts "JIT: #{jit}"
puts

# Note: split_selectors does NOT strip whitespace — that's done by parse_selectors!
# So expected results include leading spaces from ", .b" → " .b"
puts "=== Correctness ==="
methods = { "Original" => Original, "Reviewer" => ReviewerSuggestion, "Optimised" => Optimised }
methods.each do |name, mod|
  ok = TESTS.all? do |input, expected|
    result = mod.split_selectors(input)
    if result != expected
      puts "  FAIL #{name}: #{input.inspect} => #{result.inspect} (want #{expected.inspect})"
      false
    else
      true
    end
  end
  # Also verify framework selectors match Original
  SELECTORS.each do |_, (sel, _)|
    result = mod.split_selectors(sel)
    orig = Original.split_selectors(sel)
    unless result == orig
      puts "  MISMATCH #{name} on: #{sel[0..50]}..."
      puts "    got:  #{result.inspect}"
      puts "    want: #{orig.inspect}"
      ok = false
    end
  end
  puts "  #{name}: #{ok ? 'PASS' : 'FAIL'}"
end
puts

# ═══════════════════════════════════════════════════════════════════
# Benchmark
# ═══════════════════════════════════════════════════════════════════
MODS = {
  "PR#184"    => Original,
  "Reviewer"  => ReviewerSuggestion,
  "Optimised" => Optimised,
  "split(,)"  => PlainSplit,
}

N = 150_000

# Warmup
3.times { SELECTORS.each_value { |sel, _| 500.times { MODS.each_value { |m| m.split_selectors(sel) } } } }
GC.start; GC.compact rescue nil

puts "=== Benchmark: #{SELECTORS.size} real-world selectors × #{N} iterations ==="
puts
header = MODS.keys.map { |n| n.rjust(11) }.join("")
printf "%-22s %4s  #{header}  %7s\n", "Selector", "ch", "speedup"
puts "-" * (28 + MODS.size * 11 + 9)

last_fw = nil
totals = Hash.new(0.0)
speedups = []

SELECTORS.each do |label, (sel, fw)|
  if fw != last_fw
    puts "  ── #{fw} ──"
    last_fw = fw
  end

  times = {}
  MODS.each do |mn, m|
    GC.disable
    t = Process.clock_gettime(Process::CLOCK_MONOTONIC)
    i = 0; while i < N; m.split_selectors(sel); i += 1; end
    times[mn] = Process.clock_gettime(Process::CLOCK_MONOTONIC) - t
    GC.enable; GC.start
  end

  best = times.values.min
  MODS.each_key { |mn| totals[mn] += times[mn] }
  speedups << times["PR#184"] / times["Optimised"]

  cells = MODS.keys.map do |mn|
    t = times[mn]; ips = (N / t / 1000.0).round(1)
    mark = t <= best * 1.005 ? "*" : " "
    "#{mark}#{ips}k".rjust(11)
  end
  sp = (times["PR#184"] / times["Optimised"]).round(1)
  printf "  %-20s %4d %s  %6.1fx\n", label, sel.size, cells.join(""), sp
end

puts
puts "=" * 80
puts "  SUMMARY"
puts "=" * 80
puts
printf "  Aggregate wall time (%d selectors × %dk iterations):\n", SELECTORS.size, N / 1000
totals.each do |name, t|
  extra = name == "PR#184" ? "" : "  (%.1fx vs PR#184)" % [totals["PR#184"] / t]
  printf "    %-14s %7.2fs%s\n", name, t, extra
end

puts
avg_sp = (speedups.sum / speedups.size).round(1)
min_sp = speedups.min.round(1)
max_sp = speedups.max.round(1)
printf "  Optimised vs PR#184:  avg %.1fx faster  (%.1fx – %.1fx)\n", avg_sp, min_sp, max_sp

# Champion vs split(",") gap on no-paren selectors
np_sels = SELECTORS.select { |_, (s, _)| !s.include?("(") }
puts
printf "  Optimised overhead vs incorrect split(',') on %d no-paren selectors:\n", np_sels.size
np_gaps = []
np_sels.each do |_, (sel, _)|
  to = Process.clock_gettime(Process::CLOCK_MONOTONIC)
  i = 0; while i < N; Optimised.split_selectors(sel); i += 1; end
  to = Process.clock_gettime(Process::CLOCK_MONOTONIC) - to
  tp = Process.clock_gettime(Process::CLOCK_MONOTONIC)
  i = 0; while i < N; PlainSplit.split_selectors(sel); i += 1; end
  tp = Process.clock_gettime(Process::CLOCK_MONOTONIC) - tp
  np_gaps << to / tp
  GC.start
end
avg_gap = ((np_gaps.sum / np_gaps.size - 1) * 100).round(1)
printf "    %+.1f%% (correctness is essentially free)\n", avg_gap
puts

geoffyoungs · 2026-03-21T16:14:52Z

My results are as follows:

JIT: YJIT

=== Correctness ===
  Original: PASS
  Reviewer: PASS
  Optimised: PASS

=== Benchmark: 18 real-world selectors × 150000 iterations ===

Selector                 ch       PR#184   Reviewer  Optimised   split(,)  speedup
---------------------------------------------------------------------------------
  ── Bootstrap 5 ──
  bs:headings            22      448.5k    3266.6k    3307.2k   *3595.7k     7.4x
  bs:buttons            103       84.5k    2687.0k    2706.4k   *2928.8k    32.0x
  bs:grid_cols          103      105.9k    1871.4k    1876.2k   *1962.0k    17.7x
  bs:navbar             121       88.3k    5235.2k    5411.8k   *6398.2k    61.3x
  bs:reboot              39      306.2k    2253.8k    2242.9k   *2390.2k     7.3x
  ── Tailwind CSS ──
  tw:universal           20      607.7k    5978.0k    5989.2k   *7165.7k     9.9x
  tw:media               53      221.1k    2697.4k    2697.7k   *2900.9k    12.2x
  tw:inputs              41      295.3k    4156.0k    4237.9k   *4562.3k    14.4x
  tw:prose_h             95      109.3k     114.0k    1428.4k   *3219.2k    13.1x
  tw:prose_list          79      157.0k     140.5k    2595.8k   *7546.0k    16.5x
  ── normalize.css ──
  norm:buttons           56      223.5k    4734.5k    4802.8k   *5168.3k    21.5x
  ── CSS Reset ──
  reset:where_all        81      138.7k     131.5k     392.7k   *1791.8k     2.8x
  reset:is_h             27      457.8k     407.4k   *5340.9k    3882.8k    11.7x
  ── Pico CSS ──
  pico:form              93      110.7k     108.8k     997.0k   *3655.1k     9.0x
  ── Bulma ──
  bulma:buttons          78      159.4k     137.1k    1988.0k   *7534.7k    12.5x
  ── Foundation ──
  found:grid            121       80.3k    2085.2k    2174.9k   *2265.1k    27.1x
  ── Real app ──
  app:sidebar           107      109.6k    6354.3k    6424.8k   *7271.0k    58.6x
  app:theme              58      217.5k    6718.3k    6697.3k   *7618.1k    30.8x

================================================================================
  SUMMARY
================================================================================

  Aggregate wall time (18 selectors × 150k iterations):
    PR#184           17.78s
    Reviewer          6.91s  (2.6x vs PR#184)
    Optimised         1.34s  (13.3x vs PR#184)
    split(,)          0.75s  (23.8x vs PR#184)

  Optimised vs PR#184:  avg 20.3x faster  (2.8x – 61.3x)

  Optimised overhead vs incorrect split(',') on 12 no-paren selectors:
    -3.9% (correctness is essentially free)```

grosser

man this rabbit hole got deep real quick, but it's fun :D

Do you have a value for:

how fast is old vs "just COMMA_OUTSIDE_PARENS"
how fast is old vs "split for simple + just COMMA_OUTSIDE_PARENS"
how fast is old vs "split for simple + just char split"

... because I don't really like the idea of having 2 funky implementations if we can get away with just 1

grosser · 2026-03-22T19:31:29Z

lib/css_parser/rule_set.rb

+      len = selectors.bytesize
+      return [selectors] if len == 0


would remove that, not really a common case

grosser · 2026-03-22T19:32:02Z

lib/css_parser/rule_set.rb

+        when 0 # ~80% of bytes hit this: one lookup, one comparison, done
+        when 1 # comma


faster and easier to read to just do == 0x2C here ?

grosser · 2026-03-22T19:41:17Z

  Aggregate wall time (18 selectors × 150k iterations):
    PR#184           11.93s
    Optimised         1.62s  (7.4x vs PR#184)
    OptimisedRex      0.99s  (12.1x vs PR#184)
    OptimisedChar     2.23s  (5.3x vs PR#184)
    split(,)          0.54s  (22.2x vs PR#184)

so this says just rex is fastest ? (same with --yjit)

module OptimisedRex
  COMMA_OUTSIDE_PARENS = /,(?![^()]*\))/
  def self.split_selectors(selectors)
    if !selectors.include?("(")
      return selectors.split(",")
    end
    selectors.split(COMMA_OUTSIDE_PARENS)
  end
end

grosser · 2026-03-22T19:47:46Z

FYI

case str.getbyte(pos)
      when 0x2C
        if depth == 0
          results << str.byteslice(segment_start, pos - segment_start)
          segment_start = pos + 1
        end
      when 0x28 then depth += 1
      when 0x29 then depth -= 1
      when 0x2E,0x23,0x3A   then pos += 1
      end

is simpler and faster (1.96 vs 2.19)

grosser approved these changes Mar 21, 2026

View reviewed changes

grosser reviewed Mar 22, 2026

View reviewed changes

	def split_selectors(selectors)
	def split_selectors(selectors)
	return selectors.split(',') unless selectors.include?("(") # fast for simple cases

		# Split selector string on commas, but not commas inside parentheses
		# (e.g. :is(rect, circle) or :where(.a, .b) should not be split).

		when 0 # ~80% of bytes hit this: one lookup, one comparison, done
		when 1 # comma

Conversation

geoffyoungs commented Mar 20, 2026

Pre-Merge Checklist

Uh oh!

grosser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffyoungs commented Mar 21, 2026

Uh oh!

geoffyoungs commented Mar 21, 2026

Uh oh!

grosser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grosser commented Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grosser commented Mar 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

grosser commented Mar 22, 2026 •

edited

Loading