perf: optimize hot paths and fix benchmark infrastructure#4410
perf: optimize hot paths and fix benchmark infrastructure#4410matthew-dean merged 10 commits intoless:masterfrom
Conversation
Wrap bare divisions inside percentage() calls in extra parens so benchmarks work with v4's default parens-division math mode. Add --math option passthrough to benchmark-runner.js and pass --math=always in run-historical.sh for consistent cross-version results.
- Remove `extendVisitor` alias in findMatch, use `this` directly - Replace IIFE closure for functionRegistry lookup in Ruleset.eval with inline loop ~5% improvement on main benchmark (median 38.6ms → 37.1ms)
- Selector.eval: replace map() closures with pre-allocated for loops - Ruleset transformDeclaration: replace forEach with for loop - extend-visitor visitRuleset: replace forEach with for loop, cache extend and pathCount to reduce repeated property access Combined with previous commit: ~8% improvement on 104KB benchmark (median 38.6ms → 36.4ms)
- Use pnpm for v4.3+ (workspace: protocol) - Fallback tsc installation when npm can't install locally - Install runtime deps separately when npm fails due to unpublished workspace packages (@less/test-import-module) - Use last patch version of each minor release - Skip v3.13.x (broken source: missing tree/util.js)
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe pull request refines the Less.js benchmark infrastructure and compiler internals. Changes include syntactic adjustments to Less files wrapping division expressions in parentheses, enhancement of the benchmark runner to accept command-line options, substantial updates to the historical benchmark script with TypeScript fallback mechanisms and workspace handling, consolidation of benchmark results, and refactoring of internal compiler evaluation patterns in ruleset, selector, and extend-visitor modules for explicit iteration and context handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Median: 39.07ms → 34.32ms (~12% improvement) Throughput: 2,495 KB/s → 2,828 KB/s System: macbook-pro arm64
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/less/benchmark/benchmark-runner.js (1)
13-19: Regex pattern only matches lowercase option names.The pattern
/^--([a-z-]+)=(.*)$/won't match Less options with uppercase letters (e.g.,--sourceMap=true,--strictMath=true). While this works for the current use case (--math=always), it may cause confusion if other options are needed later.🔧 Suggested fix to support camelCase options
- var optMatch = process.argv[ai].match(/^--([a-z-]+)=(.*)$/); + var optMatch = process.argv[ai].match(/^--([a-zA-Z-]+)=(.*)$/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/benchmark/benchmark-runner.js` around lines 13 - 19, The arg-parsing regex only allows lowercase option names, so update the parser in the process.argv loop that builds extraOpts (the var extraOpts and optMatch usage) to accept camelCase and digits; replace the pattern /^--([a-z-]+)=(.*)$/ with one that permits letters, digits, underscores and hyphens (e.g., use a character class like [A-Za-z0-9_-] or \w plus hyphen) so options such as --sourceMap=true and --strictMath=true are matched and stored in extraOpts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/less/benchmark/benchmark-runner.js`:
- Around line 13-19: The arg-parsing regex only allows lowercase option names,
so update the parser in the process.argv loop that builds extraOpts (the var
extraOpts and optMatch usage) to accept camelCase and digits; replace the
pattern /^--([a-z-]+)=(.*)$/ with one that permits letters, digits, underscores
and hyphens (e.g., use a character class like [A-Za-z0-9_-] or \w plus hyphen)
so options such as --sourceMap=true and --strictMath=true are matched and stored
in extraOpts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d2fe84f-66bf-4a45-aef0-fe3c2a8b0a7f
📒 Files selected for processing (9)
packages/less/benchmark/benchmark-import-reference-target.lesspackages/less/benchmark/benchmark-runner.jspackages/less/benchmark/benchmark-v39.lesspackages/less/benchmark/benchmark.lesspackages/less/benchmark/results/latest/macbook-pro_arm64.jsonpackages/less/benchmark/run-historical.shpackages/less/src/less/tree/ruleset.jspackages/less/src/less/tree/selector.jspackages/less/src/less/visitors/extend-visitor.js
- Add historical benchmark data (v3.5–v4.2) to results/runs/ - Update latest/ with all versions including v4.5.0-dev optimized results - Format JSON with 2-space indentation - Update .gitignore to track runs/ (historical records belong in git)
Apple M4 Pro, arm64, Node v18/v20/v24 Key findings: - v2.4-v2.5 fastest era (~31ms median on 104KB file) - v3.10-v3.12 massive regression (3-5x slower, 126-185ms) - v4.0 recovered to ~40ms - v4.2 fastest v4.x (35.4ms) - v4.5.1 current master: 42.2ms
Reduced from 23 to 15 versions based on full benchmark data. Dropped versions with <5% difference from their predecessor: - v2.1 (broken), v2.5, v2.7 (plateau with v2.4/v2.6) - v3.6–v3.9 (all within 1ms, flat ~41ms) - v4.1 (identical to v4.0) The full set can still be run with --versions flag.
Summary
forEach/mapwithforloops in hot paths (findMatch,Selector.eval,Ruleset.eval), yielding ~8% improvement on the 104KB benchmark (median 38.6ms → 36.4ms)percentage()calls in benchmark.lessfiles for v4's defaultparens-divisionmath mode; add--math=alwayspassthrough to benchmark runner for cross-version consistencyrun-historical.sh(pnpm for workspace protocol, fallback tsc, separate runtime dep install for broken monorepo npm installs); use last patch of each minor versionBenchmark results (104KB file, Apple M4 Pro)
Test plan
grunt test)run-historical.shSummary by CodeRabbit
Release Notes
Bug Fixes
Performance