treewide: add hotpath for profiling allocations and timings
#451
Conversation
Signed-off-by: NotAShelf <[email protected]> Change-Id: I7899d1b4c8dd7dddac01c0819afa1c096a6a6964
Signed-off-by: NotAShelf <[email protected]> Change-Id: Icc47ecd660f95d1f10423a2a1bdb6cc36a6a6964
WalkthroughAdds optional hotpath instrumentation: new optional dependency and feature flags in Cargo.toml, feature-gated hotpath initialization in main, cfg-gated Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Binary as main.rs
participant Hot as hotpath (feature)
participant App as Application functions
Note over Binary,Hot #f0f7ff: Compile-time feature gating
User->>Binary: execute
alt hotpath feature enabled
Binary->>Hot: GuardBuilder::new("main").build()
Hot-->>Binary: Guard active
Binary->>App: call public methods (annotated with hotpath::measure)
App->>Hot: enter/exit measurements
Note right of App #e8f7ea: search: measure_block!("http_request") and measure_block!("json_parse")
else hotpath disabled
Binary->>App: call public methods (normal)
end
App-->>User: return results
sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Repo
participant Runner
participant Artifacts
participant CommentJob
Repo->>GH: PR opened/updated
GH->>Runner: run "Rust Profiling" workflow
Runner->>Runner: setup toolchains, run cargo with hotpath-enabled captures (head/base)
Runner->>Artifacts: upload timing & allocation JSON
GH->>CommentJob: start comment job
CommentJob->>Artifacts: download metrics
CommentJob->>CommentJob: parse & compare metrics
CommentJob->>Repo: post PR comment with comparison
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/util/platform.rs (1)
266-290: Critical: Incorrect variable reference.Line 282 references
self.passthrough, butbuild_configurationis a standalone function (not a method), andselfis not available. The correct variable ispassthrough, which was created on line 276.Apply this diff:
.extra_arg("--out-link") .extra_arg(out_path.get_path()) .extra_args(extra_args) - .passthrough(&self.passthrough) + .passthrough(&passthrough) .builder(builder) .message(message) .nom(!no_nom)
🧹 Nitpick comments (3)
src/main.rs (1)
18-18: Optional: Consider removing the redundant import.Line 27 uses the full path
hotpath::GuardBuilder::new("main"), so the import on line 18 appears unnecessary. You can safely remove it without affecting functionality.Apply this diff if you'd like to remove the redundant import:
-#[cfg(feature = "hotpath")] use hotpath; - use crate::commands::ElevationStrategy;src/search.rs (2)
150-159: Code duplication in conditional blocks.The HTTP request logic is duplicated between the
cfg(feature = "hotpath")andcfg(not(feature = "hotpath"))blocks. While this is unavoidable with the current approach, it creates a maintenance burden—if the request logic changes, both blocks must be updated identically.Consider documenting this pattern or verifying both blocks stay in sync during future changes.
185-195: Similar duplication for JSON parsing.Same concern as the HTTP request block—the JSON parsing logic is duplicated. This is a known tradeoff of the conditional compilation approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.toml(2 hunks)src/checks.rs(2 hunks)src/clean.rs(1 hunks)src/commands.rs(2 hunks)src/completion.rs(1 hunks)src/darwin.rs(1 hunks)src/home.rs(1 hunks)src/main.rs(1 hunks)src/nixos.rs(1 hunks)src/search.rs(3 hunks)src/update.rs(1 hunks)src/util.rs(2 hunks)src/util/platform.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test NH on Linux
- GitHub Check: treewide-checks
- GitHub Check: Test NH on Darwin
- GitHub Check: Build NH on Linux
- GitHub Check: Build NH on Darwin
🔇 Additional comments (17)
Cargo.toml (1)
38-38: LGTM! Clean integration of hotpath as optional dependency.The optional dependency declaration and feature flags are properly structured. The feature configuration correctly gates the instrumentation behind feature flags, ensuring zero impact when disabled.
Also applies to: 62-67
src/main.rs (1)
26-27: LGTM! Proper hotpath initialization.The guard is correctly initialized and scoped to measure the entire main function execution. The underscore prefix on
_guardappropriately indicates the variable is intentionally unused beyond its RAII lifetime.src/completion.rs (1)
14-19: LGTM! Consistent instrumentation pattern.The conditional hotpath measurement is properly applied with no impact on functionality when the feature is disabled.
src/darwin.rs (1)
32-45: LGTM! Instrumentation correctly applied.The hotpath measurement attribute follows the established pattern and is properly gated behind the feature flag.
src/nixos.rs (1)
34-52: LGTM! Consistent instrumentation.The measurement attribute is properly applied to the entry point method.
src/home.rs (1)
24-37: LGTM! Properly instrumented.The hotpath measurement follows the consistent pattern applied across the codebase.
src/util.rs (2)
144-157: LGTM! Sensible instrumentation point.Measuring
get_nix_versionis useful since it executes an external command, making it a relevant performance tracking point.
233-249: LGTM! Good choice for measurement.Instrumenting
get_nix_experimental_featuresis appropriate as it involves external command execution and is likely called during critical paths.src/clean.rs (1)
80-387: LGTM! Comprehensive instrumentation of the clean operation.The measurement attribute is appropriately placed on the main entry point for clean operations.
src/checks.rs (2)
18-19: LGTM! Clean instrumentation.The hotpath measurement attributes are correctly applied to these public functions, enabling optional performance profiling without affecting behavior when the feature is disabled.
133-134: LGTM! Consistent instrumentation pattern.The hotpath attribute follows the same pattern used throughout the codebase.
src/update.rs (1)
5-9: LGTM! Straightforward instrumentation.The hotpath attribute is correctly applied to the public
updatefunction.src/search.rs (1)
64-65: LGTM! Function-level instrumentation.The hotpath attribute correctly instruments the entire
runmethod.src/commands.rs (2)
459-460: LGTM! Instrumentation on Command::run().Appropriate placement for profiling command execution.
672-673: LGTM! Instrumentation on Build::run().Consistent with the Command::run() instrumentation.
src/util/platform.rs (2)
236-237: LGTM! Instrumentation on compare_configurations.Appropriate for profiling configuration comparison operations.
406-424: LGTM! Instrumentation on handle_rebuild_workflow.The hotpath attribute is correctly placed on this high-level workflow function, though the function signature has many parameters (allowed by the clippy exception on line 407).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/profile.yaml (1)
61-68: Minor: Variable expansion for JSON output files is fragile.While the heredoc approach should work with GitHub Actions' variable encoding, directly expanding raw JSON into files can be error-prone if the output contains special characters. Consider using a safer approach:
- name: Save metrics to artifact run: | mkdir -p /tmp/metrics cat > /tmp/metrics/head_timing.json << 'EOF' ${{ steps.head_timing_metrics.outputs.metrics }} EOF cat > /tmp/metrics/base_timing.json << 'EOF' ${{ steps.base_timing_metrics.outputs.metrics }} EOF cat > /tmp/metrics/head_alloc.json << 'EOF' ${{ steps.head_alloc_metrics.outputs.metrics }} EOF cat > /tmp/metrics/base_alloc.json << 'EOF' ${{ steps.base_alloc_metrics.outputs.metrics }} EOF echo '${{ github.event.pull_request.number }}' > /tmp/metrics/pr_number.txtThis guards against potential issues with unescaped content.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/profile.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: Test NH on Darwin
- GitHub Check: treewide-checks
🔇 Additional comments (1)
.github/workflows/profile.yaml (1)
144-150: Ensure outputs are properly exported.The
commentoutput is correctly set at lines 144-146, but as noted above,pr_numbermust also be exported to$GITHUB_OUTPUTfor line 150 to function correctly.Verify the complete output export looks like:
echo "pr_number=$PR_NUMBER" >> $GITHUB_OUTPUT echo "comment<<EOF" >> $GITHUB_OUTPUT echo "$COMMENT" >> $GITHUB_OUTPUT echo "EOF" >> $GITHUB_OUTPUT
NotAShelf
force-pushed
the
notashelf/push-nnvslnmttzkq
branch
from
6a51b95 to
915d900
Compare
Signed-off-by: NotAShelf <[email protected]> Change-Id: Iefb4347101d472b51698d78754da583d6a6a6964
NotAShelf
force-pushed
the
notashelf/push-nnvslnmttzkq
branch
from
915d900 to
88f6a62
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/profile.yaml (1)
26-65: Consider running multiple representative workloads instead of a single hardcoded search.The workflow runs only
search "nix"to gather metrics. A single query may not be representative of real-world usage or reveal performance regressions in less-exercised code paths. Consider parameterizing the workflow to run multiple commands or accepting a list of representative scenarios.Additionally, consider adding a noise filter to suppress trivial changes (e.g., < 2% variance) to reduce alert fatigue, and documenting why
search "nix"was chosen as the profiling workload.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/profile.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/profile.yaml
13-13: could not parse as YAML: did not find expected '-' indicator
(syntax-check)
🪛 YAMLlint (1.37.1)
.github/workflows/profile.yaml
[error] 18-18: syntax error: expected , but found ''
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: Test NH on Darwin
🔇 Additional comments (1)
.github/workflows/profile.yaml (1)
66-79: Quote variable expansions when writing metrics to files.Lines 69-73 use unquoted shell variable expansion. While unlikely to cause issues here, it's a potential vulnerability if the JSON output contains special shell characters. Quote the expansions for safety.
Apply this diff:
- name: Save metrics to artifact run: | mkdir -p /tmp/metrics - echo '${{ steps.head_timing_metrics.outputs.metrics }}' > /tmp/metrics/head_timing.json - echo '${{ steps.base_timing_metrics.outputs.metrics }}' > /tmp/metrics/base_timing.json - echo '${{ steps.head_alloc_metrics.outputs.metrics }}' > /tmp/metrics/head_alloc.json - echo '${{ steps.base_alloc_metrics.outputs.metrics }}' > /tmp/metrics/base_alloc.json - echo '${{ github.event.pull_request.number }}' > /tmp/metrics/pr_number.txt + echo "${{ steps.head_timing_metrics.outputs.metrics }}" > /tmp/metrics/head_timing.json + echo "${{ steps.base_timing_metrics.outputs.metrics }}" > /tmp/metrics/base_timing.json + echo "${{ steps.head_alloc_metrics.outputs.metrics }}" > /tmp/metrics/head_alloc.json + echo "${{ steps.base_alloc_metrics.outputs.metrics }}" > /tmp/metrics/base_alloc.json + echo "${{ github.event.pull_request.number }}" > /tmp/metrics/pr_number.txtLikely an incorrect or invalid review comment.
| - uses: actions/checkout@v5 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - uses: dtolnay/rust-toolchain@stable | ||
| - uses: Swatinem/rust-cache@v2 | ||
|
|
||
| - name: Install Nix | ||
| uses: cachix/install-nix-action@v31 | ||
| with: | ||
| nix_path: nixpkgs=channel:nixos-unstable | ||
|
|
There was a problem hiding this comment.
Fix YAML indentation errors in the steps block.
Lines 18 and 21 have inconsistent indentation (7 spaces instead of 6), causing YAML parsing failures. All step items under steps: must align at the same indentation level.
Apply this diff to fix the indentation:
- uses: actions/checkout@v5
with:
fetch-depth: 0
- - uses: dtolnay/rust-toolchain@stable
- - uses: Swatinem/rust-cache@v2
+ - uses: dtolnay/rust-toolchain@stable
+ - uses: Swatinem/rust-cache@v2
- - name: Install Nix
+ - name: Install Nix
uses: cachix/install-nix-action@v31
with:
nix_path: nixpkgs=channel:nixos-unstable📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v5 | |
| with: | |
| fetch-depth: 0 | |
| - uses: dtolnay/rust-toolchain@stable | |
| - uses: Swatinem/rust-cache@v2 | |
| - name: Install Nix | |
| uses: cachix/install-nix-action@v31 | |
| with: | |
| nix_path: nixpkgs=channel:nixos-unstable | |
| - uses: actions/checkout@v5 | |
| with: | |
| fetch-depth: 0 | |
| - uses: dtolnay/rust-toolchain@stable | |
| - uses: Swatinem/rust-cache@v2 | |
| - name: Install Nix | |
| uses: cachix/install-nix-action@v31 | |
| with: | |
| nix_path: nixpkgs=channel:nixos-unstable |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 18-18: syntax error: expected , but found ''
(syntax)
🤖 Prompt for AI Agents
.github/workflows/profile.yaml around lines 14 to 25: the steps block has
inconsistent indentation (lines 18 and 21 are indented one extra space), which
breaks YAML parsing; align all items under the steps: block to the same
indentation level by reducing the extra space on those lines so that each step
entry (the lines starting with "- uses:" and "- name:") shares identical
indentation, and ensure nested keys like with: and nix_path remain indented
relative to their parent step.
| - id: head_timing_metrics | ||
| run: | | ||
| # Extract timing metrics from head branch | ||
| { | ||
| echo 'metrics<<EOF' | ||
| cargo run --features='hotpath,hotpath-ci' -- search "nix" | grep '^{"hotpath_profiling_mode"' | ||
| echo 'EOF' | ||
| } >> "$GITHUB_OUTPUT" | ||
|
|
||
| - id: head_alloc_metrics | ||
| run: | | ||
| # Extract alloc metrics from head branch | ||
| { | ||
| echo 'metrics<<EOF' | ||
| cargo run --features='hotpath,hotpath-ci,hotpath-alloc-count-total' -- search "nix" | grep '^{"hotpath_profiling_mode"' | ||
| echo 'EOF' | ||
| } >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
🧩 Analysis chain
Add error handling and validation for cargo command output.
The workflow assumes cargo run --features='hotpath,hotpath-ci' -- search "nix" produces JSON with {"hotpath_profiling_mode" prefix. If this assumption is violated—due to missing features, build failure, or changed output format—grep returns empty results silently, causing later jq parsing to fail with cryptic errors.
Add explicit error handling:
- id: head_timing_metrics
run: |
# Extract timing metrics from head branch
+ set -e
{
echo 'metrics<<EOF'
- cargo run --features='hotpath,hotpath-ci' -- search "nix" | grep '^{"hotpath_profiling_mode"'
+ OUTPUT=$(cargo run --features='hotpath,hotpath-ci' -- search "nix" 2>&1) || { echo "Cargo build/run failed"; exit 1; }
+ MATCHED=$(echo "$OUTPUT" | grep '^{"hotpath_profiling_mode"' || true)
+ if [ -z "$MATCHED" ]; then
+ echo "Warning: No hotpath JSON output found. Output was: $OUTPUT" >&2
+ fi
+ echo "$MATCHED"
echo 'EOF'
} >> "$GITHUB_OUTPUT"Additionally, verify that the nh binary actually supports the search command and hotpath output format by checking documentation or running a quick test locally first.
🏁 Script executed:
#!/bin/bash
# Find and examine Cargo.toml for feature definitions
fd -e Cargo.toml | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for search command implementation and hotpath features
rg -n "hotpath" --type=toml | head -20Length of output: 581
🏁 Script executed:
#!/bin/bash
# Look for search command definition
rg -n "fn search|command.*search" --type=rs -C3 | head -40Length of output: 26
🏁 Script executed:
#!/bin/bash
# Check for any tests or examples of search output
rg -n "hotpath_profiling_mode" --type=rs -C2 | head -30Length of output: 26
🏁 Script executed:
#!/bin/bash
# Examine the actual workflow file to see full context
cat .github/workflows/profile.yamlLength of output: 5778
🏁 Script executed:
#!/bin/bash
# Search for any reference to hotpath-ci in the codebase
rg -n "hotpath-ci" -C2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for search command implementation - try without type restriction
rg -n "fn search" -C3 | head -60Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for hotpath_profiling_mode to understand output format
rg -n "hotpath_profiling_mode" -C3 | head -40Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find main.rs or CLI entry point
fd -type f -name "main.rs" -o -name "lib.rs" | head -10Length of output: 170
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation about the search command
rg -n "search.*nix" -C2 | head -30Length of output: 1361
🏁 Script executed:
#!/bin/bash
# Look at the Cargo.toml more carefully for all feature definitions
cat Cargo.toml | grep -A 10 "\[\[features\]\]" | head -30Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get full Cargo.toml features section
sed -n '/^\[features\]/,/^\[/p' Cargo.toml | head -50Length of output: 374
🏁 Script executed:
#!/bin/bash
# Search for "hotpath" in Cargo.toml to see all occurrences
grep -n "hotpath" Cargo.tomlLength of output: 382
🏁 Script executed:
#!/bin/bash
# Check git history for hotpath-ci - was it removed?
git log -p --all -S "hotpath-ci" -- Cargo.toml | head -50Length of output: 59
🏁 Script executed:
#!/bin/bash
# Look for profiling or hotpath output in source code
rg -i "profiling_mode|metrics" src/ | head -30Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are any recent workflow changes or comments about this
git log -n 10 --oneline -- .github/workflows/profile.yamlLength of output: 38
Fix undefined feature flag hotpath-ci and add error handling to cargo commands.
The workflow uses --features='hotpath,hotpath-ci' (lines 31, 40, 53, 62), but the hotpath-ci feature is not defined in Cargo.toml. Valid features are: hotpath, hotpath-alloc-bytes-total, hotpath-alloc-count-total, hotpath-alloc-self, hotpath-off. This will cause the build to fail with an "unknown feature" error.
Additionally, without error handling, cargo failures and missing output will cascade silently into jq parsing failures later in the workflow.
Corrective actions:
- Either define the
hotpath-cifeature in Cargo.toml, or replace with a valid feature combination - Add
set -eand explicit error checking to surface failures immediately:
- id: head_timing_metrics
run: |
# Extract timing metrics from head branch
+ set -e
{
echo 'metrics<<EOF'
- cargo run --features='hotpath,hotpath-ci' -- search "nix" | grep '^{"hotpath_profiling_mode"'
+ OUTPUT=$(cargo run --features='hotpath,hotpath-ci' -- search "nix" 2>&1) || { echo "Cargo failed"; exit 1; }
+ echo "$OUTPUT" | grep '^{"hotpath_profiling_mode"' || { echo "No profiling output found"; exit 1; }
echo 'EOF'
} >> "$GITHUB_OUTPUT"🤖 Prompt for AI Agents
In .github/workflows/profile.yaml around lines 26 to 42, the workflow uses a
non-existent feature flag `hotpath-ci` (causing cargo to error) and lacks error
handling for the cargo invocations; replace `--features='hotpath,hotpath-ci'`
with a valid feature set (e.g., `--features='hotpath'` for timing metrics) and
replace `--features='hotpath,hotpath-ci,hotpath-alloc-count-total'` with
`--features='hotpath,hotpath-alloc-count-total'` for alloc metrics, and add
shell error handling so failures surface immediately (enable errexit and check
cargo exit status and that the command produced output, failing the job with a
clear message if cargo or grep produce no result).
| HEAD_TIMING_MAP=$(jq -s 'map({key: .function, value: .time_ns}) | from_entries' /tmp/metrics/head_timing.json) | ||
| BASE_TIMING_MAP=$(jq -s 'map({key: .function, value: .time_ns}) | from_entries' /tmp/metrics/base_timing.json) | ||
|
|
||
| # Parse alloc metrics | ||
| HEAD_ALLOC_MAP=$(jq -s 'map({key: .function, value: .alloc_count}) | from_entries' /tmp/metrics/head_alloc.json) | ||
| BASE_ALLOC_MAP=$(jq -s 'map({key: .function, value: .alloc_count}) | from_entries' /tmp/metrics/base_alloc.json) | ||
|
|
||
| # Generate comparison table | ||
| TIMING_COMPARISON=$(jq -n --argjson head "$HEAD_TIMING_MAP" --argjson base "$BASE_TIMING_MAP" ' | ||
| ($head | keys) as $funcs | | ||
| $funcs | map({ | ||
| function: ., | ||
| head: ($head[.] // 0), | ||
| base: ($base[.] // 0), | ||
| diff: (($head[.] // 0) - ($base[.] // 0)), | ||
| pct: if ($base[.] // 0) != 0 then ((($head[.] // 0) - ($base[.] // 0)) / ($base[.] // 0) * 100) else 0 end | ||
| }) | sort_by(.pct | -.) | ||
| ') | ||
|
|
||
| ALLOC_COMPARISON=$(jq -n --argjson head "$HEAD_ALLOC_MAP" --argjson base "$BASE_ALLOC_MAP" ' | ||
| ($head | keys) as $funcs | | ||
| $funcs | map({ | ||
| function: ., | ||
| head: ($head[.] // 0), | ||
| base: ($base[.] // 0), | ||
| diff: (($head[.] // 0) - ($base[.] // 0)), | ||
| pct: if ($base[.] // 0) != 0 then ((($head[.] // 0) - ($base[.] // 0)) / ($base[.] // 0) * 100) else 0 end | ||
| }) | sort_by(.pct | -.) | ||
| ') |
There was a problem hiding this comment.
Add jq input validation and document expected JSON schema.
The jq commands assume metrics JSON has specific keys (.function, .time_ns, .alloc_count). If hotpath produces a different schema, jq will silently convert missing keys to null, producing misleading comparison tables.
Add a validation step before parsing:
- name: Generate comment
id: comment
run: |
PR_NUMBER=$(cat /tmp/metrics/pr_number.txt)
echo "pr_number=$PR_NUMBER" >> $GITHUB_OUTPUT
+ # Validate JSON structure before processing
+ for file in /tmp/metrics/{head,base}_timing.json /tmp/metrics/{head,base}_alloc.json; do
+ if ! jq -e '.[0] | has("function") and (has("time_ns") or has("alloc_count"))' "$file" > /dev/null 2>&1; then
+ echo "Error: Invalid JSON schema in $file" >&2
+ exit 1
+ fi
+ done
# Parse timing metrics
HEAD_TIMING_MAP=$(jq -s 'map({key: .function, value: .time_ns}) | from_entries' /tmp/metrics/head_timing.json)Also add inline documentation of the expected JSON format as a comment in the workflow.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .github/workflows/profile.yaml around lines 97 to 125, the jq parsing assumes
each metrics JSON contains .function, .time_ns and .alloc_count which can be
missing and produce misleading results; add a validation step that (1) asserts
the metric files exist and are non-empty, (2) runs a jq predicate against each
file to ensure every element has the expected keys and correct primitive types
(e.g. .function is string, .time_ns and .alloc_count are numbers), and fails the
job with a clear message if validation fails, and (3) only then proceeds to
build HEAD_* and BASE_* maps; also insert an inline comment above these steps
documenting the expected JSON array schema for the metrics files (example: array
of objects with keys "function":string, "time_ns":number, "alloc_count":number).
|
I wouldn't wish the YAML part on my worst enemy. I mostly copied it from hotpath repo examples, and coderabbit caught some obvious mistakes. I'll fix those tomorrow. |
2 participants
Adds optional integration with the
hotpathcrate to measure and profile the performance of key functions throughout nh. The integration is controlled through newly added Cargo features, and should not have any effects on the non-profile builds. Most of the changes are focused on annotating important functions withhotpath::measureto enable performance tracking when the feature is enabled, but I plan to add CI to run those builds automatically.Signed-off-by: NotAShelf [email protected]
Change-Id: I7899d1b4c8dd7dddac01c0819afa1c096a6a6964
Summary by CodeRabbit