Report final benchmark results as ns/op#216
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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)
📝 WalkthroughWalkthroughThis PR updates the benchmark suite to compute and report timing in nanoseconds per operation instead of milliseconds, adds optional LiveHD benchmark support via environment variables, and adjusts documentation and downstream tools to reflect these changes. ChangesBenchmark timing units and LiveHD integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Sohamkayal4103
force-pushed
the
bench_final
branch
from
fe13a7f to
7253f80
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hhds/benchmarks_final/livehd/README.md`:
- Around line 11-21: Replace the hardcoded developer path shown in README.md
("/Users/hamidsadjadpour/Desktop/masc_ucsc/livehd") with a generic example or a
description of the actual default behavior: state that LIVEHD_ROOT defaults to
the parent directory of the HHDS repo plus "/livehd" (and that you can override
it with the LIVEHD_ROOT environment variable when running run_all.sh); reference
the run_all.sh behavior (the default computed on line 10) so readers know where
the default comes from.
In `@hhds/benchmarks_final/scripts/run_all.sh`:
- Around line 37-42: The LiveHD build step currently can abort the whole script
due to set -e, so modify the block that references LIVEHD_ROOT and runs bazel
build //lgraph:livehd_final_bench to handle failures explicitly: run the build
in a subshell or capture its exit code, set LIVEHD_AVAILABLE=1 only if the build
exits with code 0, and on non-zero exit print a warning (using echo or logger)
but continue without exiting so subsequent benchmarks (HHDS/Boost) still run;
update the block that sets LIVEHD_AVAILABLE and uses LIVEHD_ROOT accordingly to
reflect this guarded build behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89ecde6f-570b-4b7b-8c55-8d509f6db5e3
📒 Files selected for processing (5)
hhds/benchmarks_final/README.mdhhds/benchmarks_final/livehd/README.mdhhds/benchmarks_final/scripts/make_comparison.pyhhds/benchmarks_final/scripts/plot_results.pyhhds/benchmarks_final/scripts/run_all.sh
| Default expected path: | ||
|
|
||
| ```text | ||
| /Users/hamidsadjadpour/Desktop/masc_ucsc/livehd | ||
| ``` | ||
|
|
||
| Override it with: | ||
|
|
||
| ```bash | ||
| LIVEHD_ROOT=/path/to/livehd bash hhds/benchmarks_final/scripts/run_all.sh | ||
| ``` |
There was a problem hiding this comment.
Replace hardcoded developer path with a generic example.
Line 14 shows an absolute path (/Users/hamidsadjadpour/Desktop/masc_ucsc/livehd) that appears to be from a specific developer's machine. This should be replaced with a generic example or removed in favor of describing the actual default behavior.
The default in run_all.sh line 10 is computed dynamically as the parent directory of the HHDS repo plus /livehd, not a hardcoded path.
📝 Suggested fix
Default expected path:
```text
-/Users/hamidsadjadpour/Desktop/masc_ucsc/livehd
+<parent-directory-of-hhds-repo>/livehd
Or describe the default behavior:
```diff
-Default expected path:
+Default behavior:
```text
-/Users/hamidsadjadpour/Desktop/masc_ucsc/livehd
+LIVEHD_ROOT defaults to the parent directory of the HHDS repo, plus /livehd
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
Default expected path:
| Default expected path: | |
| ```text | |
| /Users/hamidsadjadpour/Desktop/masc_ucsc/livehd | |
| ``` | |
| Override it with: | |
| ```bash | |
| LIVEHD_ROOT=/path/to/livehd bash hhds/benchmarks_final/scripts/run_all.sh | |
| ``` | |
| Default behavior: | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hhds/benchmarks_final/livehd/README.md` around lines 11 - 21, Replace the
hardcoded developer path shown in README.md
("/Users/hamidsadjadpour/Desktop/masc_ucsc/livehd") with a generic example or a
description of the actual default behavior: state that LIVEHD_ROOT defaults to
the parent directory of the HHDS repo plus "/livehd" (and that you can override
it with the LIVEHD_ROOT environment variable when running run_all.sh); reference
the run_all.sh behavior (the default computed on line 10) so readers know where
the default comes from.
| LIVEHD_AVAILABLE=0 | ||
| if [[ -f "$LIVEHD_ROOT/lgraph/tests/livehd_final_bench.cpp" ]]; then | ||
| echo "Building LiveHD benchmark..." | ||
| (cd "$LIVEHD_ROOT" && bazel build //lgraph:livehd_final_bench) | ||
| LIVEHD_AVAILABLE=1 | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n hhds/benchmarks_final/scripts/run_all.sh | head -50Repository: masc-ucsc/hhds
Length of output: 2264
🏁 Script executed:
# Check if set -euo pipefail is set and look at full context
cat hhds/benchmarks_final/scripts/run_all.shRepository: masc-ucsc/hhds
Length of output: 8835
🏁 Script executed:
# Also search for documentation about LiveHD being optional
find . -name "*.md" -o -name "*.txt" -o -name "*.rst" | head -20 | xargs grep -l "LiveHD" 2>/dev/null || echo "No docs found"Repository: masc-ucsc/hhds
Length of output: 133
🏁 Script executed:
cat hhds/benchmarks_final/README.md | head -100Repository: masc-ucsc/hhds
Length of output: 1574
🏁 Script executed:
cat hhds/benchmarks_final/livehd/README.mdRepository: masc-ucsc/hhds
Length of output: 646
Verify build-failure behavior is intentional.
If livehd_final_bench.cpp exists but the bazel build command fails, the script will exit entirely (due to set -euo pipefail at line 2) and prevent all benchmarks from running. This conflicts with the documented design where LiveHD is optional. The current code sets LIVEHD_AVAILABLE=1 only after a successful build, but if the build fails, the script exits before that flag is ever checked, blocking HHDS and Boost benchmark execution.
Consider wrapping the build in explicit error handling to allow the script to continue when LiveHD build fails:
Suggested fix
LIVEHD_AVAILABLE=0
if [[ -f "$LIVEHD_ROOT/lgraph/tests/livehd_final_bench.cpp" ]]; then
echo "Building LiveHD benchmark..."
- (cd "$LIVEHD_ROOT" && bazel build //lgraph:livehd_final_bench)
- LIVEHD_AVAILABLE=1
+ if (cd "$LIVEHD_ROOT" && bazel build //lgraph:livehd_final_bench); then
+ LIVEHD_AVAILABLE=1
+ else
+ echo "Warning: LiveHD build failed; continuing without LiveHD benchmarks."
+ fi
fi📝 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.
| LIVEHD_AVAILABLE=0 | |
| if [[ -f "$LIVEHD_ROOT/lgraph/tests/livehd_final_bench.cpp" ]]; then | |
| echo "Building LiveHD benchmark..." | |
| (cd "$LIVEHD_ROOT" && bazel build //lgraph:livehd_final_bench) | |
| LIVEHD_AVAILABLE=1 | |
| fi | |
| LIVEHD_AVAILABLE=0 | |
| if [[ -f "$LIVEHD_ROOT/lgraph/tests/livehd_final_bench.cpp" ]]; then | |
| echo "Building LiveHD benchmark..." | |
| if (cd "$LIVEHD_ROOT" && bazel build //lgraph:livehd_final_bench); then | |
| LIVEHD_AVAILABLE=1 | |
| else | |
| echo "Warning: LiveHD build failed; continuing without LiveHD benchmarks." | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hhds/benchmarks_final/scripts/run_all.sh` around lines 37 - 42, The LiveHD
build step currently can abort the whole script due to set -e, so modify the
block that references LIVEHD_ROOT and runs bazel build
//lgraph:livehd_final_bench to handle failures explicitly: run the build in a
subshell or capture its exit code, set LIVEHD_AVAILABLE=1 only if the build
exits with code 0, and on non-zero exit print a warning (using echo or logger)
but continue without exiting so subsequent benchmarks (HHDS/Boost) still run;
update the block that sets LIVEHD_AVAILABLE and uses LIVEHD_ROOT accordingly to
reflect this guarded build behavior.
1 participant
Summary
This PR updates the final benchmark reporting to use
ns/opinstead of total wall-clock time.Changes
wall_ns / itemsns/opacross repeated runsns/opWhy
ns/opis easier to compare across different graph sizes because it normalizes total runtime by the number of operations performed.Notes
Raw benchmark CSVs still keep
wall_nsanditems. The normalizedns/opvalues are generated in the comparison CSV and plots.Summary by CodeRabbit
Documentation
Improvements