Make xprofile time/memory measured and bottlenecks structured#87
Conversation
time_ms and memory_mb were heuristic estimates (time_ms could round to 0;
memory_mb was a fixed-base formula). Parse the cachegrind `summary:` line for
the real measured totals (units from the `events:` header, Time_(10ns) and
Memory_(bytes)), keeping the estimate only as a fallback.
bottlenecks were preformatted strings like "main (54%)". Emit a structured
array [{function, percentage, time_ms}] so consumers can jq-filter/sort.
Also fix a latent bug: per-function cost summed the line number (1st cost
column) instead of the Time column (2nd), so percentages were meaningless.
Now percentages and time_ms are time-based, and the bottleneck time_ms values
sum to the reported total.
Schema, MCP tool description, skill, and TROUBLESHOOTING jq example follow.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Warning Review limit reached
More reviews will be available in 43 minutes and 5 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughxprofile now reads measured execution time and peak memory from Cachegrind summary data, accumulates function time from the correct cost column, and emits structured bottleneck objects. The schema and related docs were updated to match the new output shape. ChangesXprofile output and contract updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 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 `@bin/xprofile`:
- Around line 395-412: The bottleneck output format in xprofile was changed to
an array of structured entries, but the default CLI rendering still treats each
item as a string. Update displayHumanReadableProfile() to detect and render the
bottleneck_functions entries as objects/arrays (showing function, percentage,
and time_ms when present) instead of echoing them directly, so the top functions
remain readable in the default CLI path.
- Around line 250-262: The Cachegrind parsing in xprofile hard-codes Time_(10ns)
and Memory_(bytes) column positions, which breaks when xdebug.cachegrind_events
changes the event order. Update the parsing logic to read the events: header
first, map the indices for Time_(10ns) and Memory_(bytes), and then use those
indices consistently in the summary processing and cost-line aggregation code
paths. Use the existing xprofile Cachegrind parsing section and the
summary/cost-line handling logic as the main places to change.
In `@src/McpServer.php`:
- Line 121: The runtime metadata for the profile performance tool is advertising
the wrong contract key: the description in McpServer and the mirrored wording in
skills/xdebug/SKILL.md mention a $schema URL, but bin/xprofile actually emits
schema. Update the text in the relevant metadata/description strings to
advertise the actual schema field name consistently, using the
profile/performance description and any mirrored skill docs so MCP clients see
the correct payload shape.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc37a586-ca8c-41d0-ba23-b343e99ba246
📒 Files selected for processing (5)
bin/xprofiledocs/TROUBLESHOOTING.mddocs/schemas/xprofile.jsonskills/xdebug/SKILL.mdsrc/McpServer.php
- Read the cachegrind `events:` header to locate the Time/Memory columns instead of hard-coding positions, so a non-default xdebug.cachegrind_events order works. The summary parse and the cost-line aggregation both use the mapped indices. - Render structured bottlenecks in the human-readable CLI path (displayHumanReadableProfile was echoing the arrays as "Array"). - Fix the profile tool description: xprofile emits `schema`, not `$schema` (McpServer description + mirrored SKILL wording).
1 participant
Summary
xprofile's
time_ms/memory_mbwere heuristic estimates andbottleneckswas a preformatted string array. This makes the metrics real measurements from the cachegrindsummary:line and structures bottlenecks for machine processing.Independent of the xstep slim-down (#86) — a separate concern (accuracy/quality, not size), branched off 1.x.
What changed
Measured time/memory (was estimated)
The cachegrind file carries
summary: <total_time> <total_memory>with units declared in theevents:header (Time_(10ns) Memory_(bytes)). We now parse those for the real totals, keeping the old heuristic only as a fallback when the summary or its units are missing.Structured bottlenecks (was string array)
["main (54%)"]becomes[{"function":"main","percentage":54.0,"time_ms":0.07}], so consumers canjq-filter/sort by percentage or time.Latent bug fixed
Per-function cost summed the line number (1st cost column) instead of the Time column (2nd), making percentages meaningless. Fixed to sum Time — now percentages and
time_msare time-based, and the bottlenecktime_msvalues sum to the reported total.Measured before / after
Same command (
xprofile --json -- php tests/fixtures/debug_test.php):Verification
Note
src/XdebugProfiler.phpand two profile DTOs are dead code (never instantiated; both CLI and MCP go throughbin/xprofile) and also mis-parse the summary line. Their removal is tracked separately, out of scope here.Summary by CodeRabbit
New Features
Bug Fixes
Documentation