Skip to content

Avoid quadratic copying in Table::getDisplayLines() - #202

Merged
swissspidy merged 1 commit into
mainfrom
claude/table-display-lines-perf
Oct 5, 2026
Merged

swissspidy merged 1 commit into
mainfrom
claude/table-display-lines-perf

Conversation

@swissspidy

@swissspidy swissspidy commented Oct 5, 2026 •

Copy link
Copy Markdown
Member

Table::getDisplayLines() merged each rendered row into the output with $out = array_merge( $out, $row ). That copies the entire output array for every row, so rendering takes quadratic time in the number of rows.

This shows up in WP-CLI's --format=table for large result sets. On a test site with ~96,600 posts, wp post list --format=table --fields=ID,post_title took 46 s, compared to ~7 s for --format=csv with the same data. With this change it takes 7.4 s, and the output is byte-identical.

The fix appends each rendered line to $out instead.

Measurements

Rendering a two-column table with getDisplayLines(), PHP 8.3:

Rows Before After
10,000 0.25 s 0.07 s
20,000 0.85 s 0.14 s
40,000 4.08 s 0.26 s

Testing

Added test_display_lines_keep_row_order_with_many_rows. It renders 5,000 rows, every 100th of which spans two lines, and checks the line count, the order of the rows and the placement of the wrapped lines. The full PHPUnit suite passes. PHPCS reports no new violations in the touched files.

Found while profiling WP-CLI for large lists (wp-cli/ideas#81, wp-cli/ideas#91).

🤖 Generated with Claude Code

https://claude.ai/code/session_01D26yjkN2BiqCXT6p6o1WqS


Generated by Claude Code

Summary by CodeRabbit

  • Performance
    • Large tables now render rows with less intermediate processing, which may improve rendering efficiency. The displayed content and row order remain unchanged.
  • Tests
    • Added coverage for large tables, including multiline values, row ordering, and table borders.

Merging every rendered row into the output with array_merge() copied the
whole output array once per row, so rendering a table took quadratic time
in the number of rows (about 4 s for 40,000 rows, and ~50 s for a
`wp post list` of ~100k posts). Append the lines instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D26yjkN2BiqCXT6p6o1WqS
Copilot AI balanced review requested due to automatic review settings October 5, 2026 18:56
@swissspidy
swissspidy requested a review from a team as a code owner October 5, 2026 18:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 207dc275-df1b-42d9-9844-c72f539ac37f
📥 Commits

Reviewing files that changed from the base of the PR and between c081843 and ea83d53.

📒 Files selected for processing (2)
  • lib/cli/Table.php
  • tests/Test_Table.php

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

getDisplayLines() now appends each rendered row line directly to the output array. A regression test checks ASCII output for 5,000 rows, including multiline values.

Changes

Table rendering

Layer / File(s) Summary
Append rendered lines and verify table output
lib/cli/Table.php, tests/Test_Table.php
getDisplayLines() appends rendered row lines directly to the output array. A regression test checks line count, row order, multiline values, and borders for 5,000 rows.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Suggested reviewers: mrsdizzie, schlessera

Merge Risk: ⚪ Minimal · up to ea83d

The supplied evidence identifies no unresolved behavior or merge-blocking risk in this localized rendering optimization.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: avoiding quadratic copying in Table::getDisplayLines().
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Oct 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@swissspidy swissspidy added this to the 0.13.1 milestone Oct 5, 2026
@swissspidy
swissspidy merged commit c7132ca into main Oct 5, 2026
25 checks passed
@swissspidy
swissspidy deleted the claude/table-display-lines-perf branch October 5, 2026 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants