Docs: Node comparison report#7346
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded a Node Comparison documentation section with Forest versus Lotus feature, RPC performance, and snapshot generation comparisons. ChangesNode comparison documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/docs/users/reports/node_comparison.md (1)
1-132: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun Prettier before merging.
Docs Check / Checkcurrently fails because this file is not formatted according to Prettier. Run Prettier with--write, then rerun the documentation checks.🤖 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 `@docs/docs/users/reports/node_comparison.md` around lines 1 - 132, Format the Node Comparison documentation with Prettier using write mode, then rerun the documentation checks and ensure the file passes without formatting errors.Source: Pipeline failures
🤖 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 `@docs/docs/users/reports/node_comparison.md`:
- Around line 121-123: Update the snapshot compression note to distinguish Lotus
export output in this benchmark from Lotus snapshots generally. Remove the claim
that Lotus snapshots are uncompressed, and align the wording with
snapshot_service.md by stating that snapshots are zstd-compressed and readable
by both Forest and Lotus.
- Line 11: Update the version references in the node comparison documentation so
the environment versions and snapshot benchmark versions are aligned, or
explicitly label the snapshot figures as a separate historical benchmark with
Forest 0.33.6 and Lotus 1.36.0. Ensure the 7–13x conclusion clearly refers to
the correct releases.
---
Outside diff comments:
In `@docs/docs/users/reports/node_comparison.md`:
- Around line 1-132: Format the Node Comparison documentation with Prettier
using write mode, then rerun the documentation checks and ensure the file passes
without formatting errors.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 66f9a67b-da56-4636-9488-086ade311903
⛔ Files ignored due to path filters (16)
docs/static/img/reports/node_comparison/average-amortized-latency.pngis excluded by!**/*.pngdocs/static/img/reports/node_comparison/average-batch-size.pngis excluded by!**/*.pngdocs/static/img/reports/node_comparison/build-info.pngis excluded by!**/*.pngdocs/static/img/reports/node_comparison/cpu.pngis excluded by!**/*.pngdocs/static/img/reports/node_comparison/in-flight-requests.pngis excluded by!**/*.pngdocs/static/img/reports/node_comparison/memory.pngis excluded by!**/*.pngdocs/static/img/reports/node_comparison/p50-amortized-latency.pngis excluded by!**/*.pngdocs/static/img/reports/node_comparison/p50-per-flow-latency.pngis excluded by!**/*.pngdocs/static/img/reports/node_comparison/p95-amortized-latency.pngis excluded by!**/*.pngdocs/static/img/reports/node_comparison/p95-per-flow-latency.pngis excluded by!**/*.pngdocs/static/img/reports/node_comparison/p99-amortized-latency.pngis excluded by!**/*.pngdocs/static/img/reports/node_comparison/p99-per-flow-latency.pngis excluded by!**/*.pngdocs/static/img/reports/node_comparison/request-count-batch-aware.pngis excluded by!**/*.pngdocs/static/img/reports/node_comparison/request-count-http.pngis excluded by!**/*.pngdocs/static/img/reports/node_comparison/snapshot-export-comparison.pngis excluded by!**/*.pngdocs/static/img/reports/node_comparison/wall-time-per-http-request.pngis excluded by!**/*.png
📒 Files selected for processing (2)
docs/docs/users/reports/node_comparison.mdpackage.json
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/docs/users/reports/node_comparison.md (1)
120-129: 🗄️ Data Integrity & Integration | 🟠 MajorScope the snapshot compression claim to the measured export.
“Lotus snapshots are also not compressed” overgeneralizes the benchmark. State whether each measured export is compressed or uncompressed, and avoid presenting the disk-space difference as an intrinsic Lotus-versus-Forest property. If this benchmark measured uncompressed Lotus export output, say so explicitly.
This follows the linked Lotus findings that compressed CAR inputs are supported.
Also applies to: 136-136
🤖 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 `@docs/docs/users/reports/node_comparison.md` around lines 120 - 129, Update the note in the snapshot comparison to explicitly state that the measured Lotus exports produced uncompressed output, and clarify that the disk-space difference applies only to this benchmark rather than being an intrinsic implementation property. Retain the distinction that Forest compresses snapshots during export and that both implementations can consume compressed snapshots.Source: Linked repositories
🤖 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 `@docs/docs/users/reports/node_comparison.md`:
- Line 8: Expand the node comparison report beyond performance, resource usage,
and snapshots by adding a compact comparison section covering implementation
language, supported features, and other defining properties requested by issue
`#5197`. Alternatively, explicitly narrow the report’s stated scope to exclude
those comparisons.
- Line 120: Update the snapshot benchmark paragraph and its associated table to
align the cited Lotus and Forest versions with the environment versions, or
explicitly label the results as historical benchmarks. Ensure the “7–13x”
performance takeaway preserves and clearly attributes the actual benchmark
versions, including the matching references around the repeated section.
---
Duplicate comments:
In `@docs/docs/users/reports/node_comparison.md`:
- Around line 120-129: Update the note in the snapshot comparison to explicitly
state that the measured Lotus exports produced uncompressed output, and clarify
that the disk-space difference applies only to this benchmark rather than being
an intrinsic implementation property. Retain the distinction that Forest
compresses snapshots during export and that both implementations can consume
compressed snapshots.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 47426223-1377-42db-982b-f94b7b6496cd
⛔ Files ignored due to path filters (1)
docs/static/img/reports/node_comparison/p50-per-flow-latency.pngis excluded by!**/*.png
📒 Files selected for processing (1)
docs/docs/users/reports/node_comparison.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
LesnyRumcajs
left a comment
LesnyRumcajs
left a comment
There was a problem hiding this comment.
Please re-read the task description. It's fine to shill Forest's performance but this was not the gap in documentation we want to close.
Let's:
- make it less marketing-aggressive. There are certain individuals in the ecosystem that do that already (especially with the help of their sycophant AI companions) and we shouldn't follow their example. Just showing the numbers and explaining them in boring, academic voice is fine.
- split to two reports - RPC and snapshot generation.
- this does not close the linked issue - a separate page must be made to address it - with actual feature objective comparison etc., good and bad points. All in all, a new user must be able to decide whether to use Forest or Lotus after reading it.
Got it. Thanks for the clarification and feedback here I'll make the changes. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted filessee 9 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
18d1b13 to
68b060e
Compare
LesnyRumcajs
left a comment
LesnyRumcajs
left a comment
There was a problem hiding this comment.
It makes sense to run any descriptions through some Grammarly to tweak up the language.
2 participants
Summary of changes
Changes introduced in this pull request:
Node Comparisionsection underReports.Reference issue to close (if applicable)
Closes #5197
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit