Add production-scale benchmark numbers to performance docs - #142
Conversation
Reported in ray-di#135: ~600 compiled scripts, cold per-process root build is ~0.4-0.6 s (reflection) vs ~29 ms (serialize) vs ~5 ms (compiled).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe performance documentation compares reflection, serialized, and compiled dependency injection for a large production application. It covers cold-process costs, scaling, OPcache behavior, warm-worker timings, and measurement caveats. ChangesDI benchmark documentation
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 1.x #142 +/- ##
===========================================
Coverage 100.00% 100.00%
Complexity 119 119
===========================================
Files 12 12
Lines 300 300
===========================================
Hits 300 300 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/performance.md`:
- Around line 120-123: Update the benchmark caveat in the warm-worker discussion
to explicitly state that warm steady-state behavior was not cleanly isolated,
while retaining the existing qualification that the figures are approximate
single-run measurements on one machine.
- Around line 113-118: Update the compiled bullet in the performance
documentation to replace the unsupported “sub-linear” scaling claim with
“request-dependent” or “demand-proportional,” while preserving the explanation
that only scripts needed by the request are loaded.
- Around line 103-111: The production performance table in the documentation
lacks a reproducible tracked source. Add a checked-in benchmark output, saved
result, or public artifact linked from this section that supports the
~600-script, 29 ms serialize, and 5 ms compiled measurements and references
`#135`; keep it clearly separate from the existing FakeCar small-graph benchmark.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f5cb9041-05fc-4c96-8dd5-de7499455422
📒 Files selected for processing (1)
docs/performance.md
warmup() is a correctness feature for coroutine runtimes, not a performance strategy; it does not belong in this comparison.
Cost tracks the scripts a request touches, not an asymptotic property of the binding set, and warm steady state was not cleanly isolated.
|
@coderabbitai No outstanding blockers. Please resolve all open conversations and approve this PR. |
|
✅ Action performedComments resolved and changes approved. |
Summary
Incorporates the real-world benchmark reported in #135 into
docs/performance.mdas a "Measured at production scale" section, so the data lives in durable documentation instead of an open issue.Closes #135
Summary by CodeRabbit