Speedup devtool checkstyle#5853
Merged
Manciukic merged 6 commits intoApr 29, 2026
Merged
Conversation
Replace per-field grep in test_unused_metrics (one subprocess per metric field, ~208 invocations) with a single grep that matches all field names at once and filters results in Python. Changes in fcmetrics.py: - Remove is_metric_used() which ran grep -RPzo per field - Add find_unused_metrics() which builds a combined alternation of all field names and runs a single grep -rPn across src/ - Filter grep output in Python to skip test files and metrics definition files (via renamed is_file_test_or_definition), comment lines, and string literals - Rename is_file_production to is_file_test_or_definition to reflect what it actually checks Changes in test_rust.py: - Simplify test_unused_metrics to call find_unused_metrics() directly - Remove unused imports (defaultdict, extract_fields, find_metrics_files, is_metric_used) Signed-off-by: Riccardo Mancini <mancio@amazon.com>
test_markdown_style was spawning a bash+diff+mdformat subprocess for each of the 66 markdown files individually (23s). Pass all files to mdformat --check in one call (~4s). Signed-off-by: Riccardo Mancini <mancio@amazon.com>
checkstyle ran three separate cmd_test invocations (style, doctests, clippy), each launching a container, copying 2.3GB of artifacts, and starting pytest. Style checks don't need artifacts at all. - Add FC_TEST_SKIP_ARTIFACT_COPY=1 support to test.sh - Set it in cmd_checkstyle to skip the artifact copy - Merge the three cmd_test calls into a single pytest invocation Signed-off-by: Riccardo Mancini <mancio@amazon.com>
Manciukic
force-pushed
the
speedup-checkstyle
branch
from
577fb02 to
2b1cd10
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5853 +/- ##
=======================================
Coverage 82.87% 82.87%
=======================================
Files 276 276
Lines 29728 29728
=======================================
Hits 24637 24637
Misses 5091 5091
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Manciukic
added
the
Status: Awaiting review
JackThomson2
approved these changes
Apr 28, 2026
ShadowCurse
approved these changes
Apr 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
The checkstyle step had several bottlenecks from spawning excessive subprocesses and doing redundant work. These changes reduce wall-clock time by consolidating them.
Unused metrics check (
fcmetrics.py,test_rust.py) —test_unused_metricsspawned onegrep -RPzoper metric field (~208 subprocesses, each scanning the entiresrc/tree). Replaced with a singlegrep -rPnthat matches all field names at once, with Python-side filtering to exclude comments, string literals, and test/definition files. Also renamed the misleadingis_file_productiontois_file_test_or_definition.Markdown formatting (
test_markdown.py) —test_markdown_styleranmdformat+diffper file. Replaced with a singlemdformat --checkinvocation for all files.CI orchestration (
devtool,test.sh) — Consolidated three separatecmd_testinvocations (style tests, doctests, clippy) into one pytest run. AddedFC_TEST_SKIP_ARTIFACT_COPYto skip copying build artifacts for style-only tests that don't need them.Before:
After:
Reason
Make checkstyle faster to speed up automated rebases.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
tools/devtool checkbuild --allto verify that the PR passesbuild checks on all supported architectures.
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.