Repository navigation
Make Declarations#to_s ~24x faster: its clean-up regexp matched at every character - #193
Conversation
… every character `[\n\r\f\t]*` matches the empty string, so gsub! performed a replacement at every position of the declarations string. Leftmost alternation also made the trailing `\s+$` branch unreachable. `+` with the dead branch dropped gives the same output (fuzzed on 200k random strings) ~24x faster.
normalize_property returned a fresh downcased copy on every lookup, even for the already normalized literals used internally (margin-top, ...). Skip the copy when the name is already lowercase ASCII. create_dimensions_shorthand! built two enumerators and a pair array per direction; fill a plain hash.
czj
changed the title
| attr_writer :declarations | ||
|
|
||
| def normalize_property(property) | ||
| return property if property.is_a?(String) && property.match?(NORMALIZED_PROPERTY_RX) |
There was a problem hiding this comment.
can we move the property = property.to_s up to avoid the string check and use the match more often ?
There was a problem hiding this comment.
Good idea, done in 7ec0f07: the property goes through to_s first, so symbols like :color now take the fast path too instead of only strings. Thanks!
| end | ||
| # TODO: Clean-up regexp doesn't seem to work | ||
| str.gsub!(/^[\s^({)]+|[\n\r\f\t]*|\s+$/mx, '') | ||
| str.gsub!(/^[\s^({)]+|[\n\r\f\t]+/, '') |
There was a problem hiding this comment.
this used to remove trailing whitespaces, don't we need that ?
There was a problem hiding this comment.
hmm "since the leftmost alternative always wins, the trailing \s+$ branch can never match"
str.gsub!(/^[\s^({)]+|[\n\r\f\t]*|\s+$/mx, '')
the problem is the middle, it matches every string, so that end never worked
There was a problem hiding this comment.
so yeah this should work and we did not really need the /m or /x either
There was a problem hiding this comment.
Exactly, the * in the middle matched the empty string everywhere, so \s+$ never got a chance; the str.strip! right after is what actually removes the trailing whitespace. And yes, the /m and /x flags are already gone in this version. Thanks a lot for the careful review, and for maintaining css_parser!
grosser
left a comment
grosser
left a comment
There was a problem hiding this comment.
It looks pretty good, not sure if it hits all the previous edge-cases
|
To follow up on "not sure if it hits all the previous edge-cases": I ran old vs new side by side on 1,000,000 random strings mixing |
|
lmk if you want a new release or if there is more coming |
|
More coming! I found more allocation cuts across parsing, shorthands and merge: −40% objects when inlining our emails, −48% on Mastodon's mailer CSS, output unchanged. I'll send them as 2 small PRs that are easy to review, over the next day or two: one for declarations, where most of the gain is, and one for shorthands and media types. Release now or wait for them, both are fine with me. And if you'd rather we stop here, just say so, no hard feelings at all :) |
|
Here they are: #194 for declarations and #195 for shorthands and media types. They touch different methods and can be merged in any order, only CHANGELOG.md will conflict and I'll rebase whichever lands last. That's all from me, so a release once they're in (or whenever suits you) would be perfect. Thanks again! |
RuleSet::Declarations#to_scleans its output withgsub!(/^[\s^({)]+|[\n\r\f\t]*|\s+$/mx, '').[\n\r\f\t]*matches the empty string, sogsub!performs a replacement at every position of the string, and since the leftmost alternative always wins, the trailing\s+$branch can never match. That's probably what the# TODO: Clean-up regexp doesn't seem to workabove it was about :)The first commit uses
/^[\s^({)]+|[\n\r\f\t]+/instead: same output on 700,000 random strings, ~24x faster per call. Through premailer, it was 74% of allgsub!time and 11.6% of the inlining wall time on our order confirmation email.The second commit is smaller:
normalize_propertyskips the downcased copy when the name is already lowercase ASCII (the internalmargin-top-style literals always are), andcreate_dimensions_shorthand!fills a plain hash instead of chaining enumerators. Happy to split it out if you'd rather review them separately.On our 156 transactional emails, inlined with premailer, output byte for byte identical before and after:
The suite passes (246 runs), RuboCop is clean.
Pre-Merge Checklist
🤖 Generated with Claude Code, but prompted, read, and verified by @czj, a caring human :)