Repository navigation
perf: speed up the worker, fix a long-line timeout, make the benchmark like for like - #86
Conversation
…t from the line start per item
… CPU on large plugins
…nes from the start of the file
…efix copy and the per-character trim
…er lines with LineIndex
… rules that need it
…dges, before a line break or empty lines
…ich port 183 of its 186 sniffs
… and regenerate the charts
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 Walkthrough
Merge Risk: 🔵 Low · up to
|
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
fe2dc9bc-583d-4843-8d44-383326cfc7a7
⛔ Files ignored due to path filters: 2
docs/benchmarks-dark.svgis excluded by!**/*.svgdocs/benchmarks-light.svgis excluded by!**/*.svg
📒 Files selected for processing: 40
README.mdbench/bakeoff.shbench/lib.shbench/results/2026-10-20-bakeoff.mdbench/run.shbin/generate-presets.phpdocs/benchmarks.mdsrc/Internal/BlankLinesAfterComment.phpsrc/Internal/Calls.phpsrc/Internal/DocBlockScan.phpsrc/Internal/FileTokens.phpsrc/Internal/ItemShift.phpsrc/Internal/JoinedStatement.phpsrc/Internal/LeadingComment.phpsrc/Internal/LineIndents.phpsrc/Internal/PhpText.phpsrc/Internal/PhpcsComments.phpsrc/Internal/ScopeColon.phpsrc/Internal/Strings.phpsrc/Linter/Rules/ArrayAlignmentRule.phpsrc/Linter/Rules/ClassCommentRule.phpsrc/Linter/Rules/ClassInstantiationRule.phpsrc/Linter/Rules/DisallowInlineTabsRule.phpsrc/Linter/Rules/DisallowLonelyIfRule.phpsrc/Linter/Rules/DisallowMultipleStatementsRule.phpsrc/Linter/Rules/EmptyCatchCommentRule.phpsrc/Linter/Rules/EndFileNewlineRule.phpsrc/Linter/Rules/EscapeOutputRule.phpsrc/Linter/Rules/FileCommentRule.phpsrc/Linter/Rules/FileHeaderRule.phpsrc/Linter/Rules/FunctionCallSignatureRule.phpsrc/Linter/Rules/FunctionCommentRule.phpsrc/Linter/Rules/InlineControlStructureRule.phpsrc/Linter/Rules/NonExecutableCodeRule.phpsrc/Linter/Rules/SeparateFunctionsFromOORule.phpsrc/Linter/Rules/SuperfluousWhitespaceRule.phpsrc/WordPressExtension.phpwordpress-core.mago.tomlwordpress-extra.mago.tomlwordpress.mago.toml
💤 Files with no reviewable changes: 1
- src/Internal/PhpText.php
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
| echo "| Tool | Mean of 3 runs |" | ||
| echo "|:---|---:|" | ||
| timed "phpcs WordPress-Extra (WPCS 3.4.1, --parallel=8)" phpcs | ||
| timed "phpcs WordPress (WPCS 3.4.1, --parallel=8)" phpcs |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy lift
Alternate the tool order during timed samples.
This call starts every PHPCS sample before any Mago sample for the same codebase. Fixed order can make cache state, temperature, or CPU clock changes appear as a tool difference. Alternate the order across samples and record the executed order with the timings. (raw.githubusercontent.com) Based on learnings, fixed sequential benchmark order can systematically bias a comparison through cache, thermal, and clock-frequency drift.
Source: Learnings
There was a problem hiding this comment.
Not changing this. Each tool gets a warm-up run and the mean of three timed runs, so cache state is the same for both, and the machine ran the same background load throughout. The order is fixed so the logged output matches the old results files.
There was a problem hiding this comment.
@rlorenzo Thanks for the clarification. The fixed order preserves the layout of earlier results files. I accept your decision to keep it.
The order-bias concern is a benchmark-methodology caveat, not a demonstrated defect in these results. I won’t press for a change.
✏️ Learnings added
Learnt from: rlorenzo
Repo: rlorenzo/mago-wordpress PR: 86
File: bench/run.sh:115-115
Timestamp: 2026-10-20T12:16:42.185Z
Learning: In rlorenzo/mago-wordpress, bench/run.sh intentionally benchmarks PHPCS before Mago so the logged output matches earlier results files. Each tool receives one warm-up run followed by three timed runs. Preserve this intentional ordering unless the benchmark requirements change.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
…Mago 1.47's analyzer reads as never from a static local
…ocs sniffs, in the timed rules
…es (1.8× overall)
What
Speeds up the PHP worker, fixes a timeout on very long single-line arrays, and makes the benchmark compare like with like. Behaviour is unchanged: the per-rule finding counts on all 11 benchmark codebases are identical to v1.6.0, and
just checkpasses on every commit.Why
Running the bake-off on v1.6.0 failed on Google Site Kit:
wordpress/array-alignmenttook 26-32 s on the Symfony polyfill data files (21k-character single-line arrays), past Mago's 30 s worker timeout. Profiling the worker found the rest of the overhead and a benchmark that did not match what it claimed.Commits (one per improvement)
fix(array-alignment): measure a long line's columns incrementally. 26 s to 0.2 s on the polyfill files.perf(presets): start the worker withopcache.enable_cli=1. 8-15% less CPU on large plugins; harmless without opcache.perf(rules),perf(strings),perf(leading-comment),perf(phpcs-comments),perf(class-comment, file-comment): replacesubstr_countfrom byte 0,strrpos(substr(...))prefix copies and a linear comment scan withLineIndex, a negative-offsetstrrposand a map by end offset. Mostly matters on large files.perf(calls),perf(empty-catch-comment),perf(escape-output),perf(superfluous-whitespace): unwrap one argument, skip files with nothing to check, reuse the shared token stream.superfluous-whitespaceis a conservative pre-check; its reports are identical on all 11 codebases (420 lines).refactor(rules): one sharedPhpToken::tokenizeof the file (FileTokens::raw) for the rules that tokenized it separately.bench,docs: phpcs runs with opcache too and with the fullWordPressstandard (the extension ports 183 of its 186 sniffs, so that is the matching comparison); results and charts regenerated.Numbers
CPU time against v1.6.0 (opcache plus the code changes): Yoast SEO 20.6 s to 18.0 s, WooCommerce 106 s to 94 s, Elementor 32 s to 28 s, about 12%. Most of that is opcache.
Bake-off, phpcs
WordPressagainst mago + extension: 92.6 s vs 52.5 s overall (1.8x); 2.1-2.7x on the large plugins, about even on the small ones.The old README figure (2-12x, 8.1x overall) came from a benchmark that ran only the 46 rules the extension had then. It now runs all 186, so the README and
docs/benchmarks.mdare updated to the measured numbers.Not done
Profiling shows no single hotspot left: shared structure builds are about 27% of rule time and the rest is spread over ~185 rules. Merging the four phpcs-style tokenizers would save roughly 10% but risks WPCS parity, so it is left for a separate change.
Review
CodeRabbit's
missing-docsfinding is fixed: the timed rules now include Mago's ownmissing-docs, which covers the WordPress-Docs sniffs. The suggestion to alternate tool order is not taken: both tools get a warm-up and three runs, and the machine ran other work equally for both. CI on Mago 1.47.1 failed on anarray-alignmentstatic local that the older analyzer read asnever; it is a typed property now.