perf: absorb column widths instead of re-measuring every row - #211
Open
hellices wants to merge 2 commits into
Open
perf: absorb column widths instead of re-measuring every row#211hellices wants to merge 2 commits into
hellices wants to merge 2 commits into
Conversation
Textual computes column widths from `on_idle` by rebuilding every new row's renderables and measuring each cell. Seeding a 50,000-row view therefore cost 700,000 renderable constructions and 700,000 measurements after the rows were already in the table -- ~74% of a 5s freeze, and the same bill on every rebuild path (kind switch, sort change, custom view, and the reorder fallback). ResourceTable already holds exactly those cells (`_emitted`, #208) and already has `_cell_width()`, written to match DataTable's measurement. Fold the emitted cells into `column.content_width` directly and hand `_update_dimensions` only the rows that were not accounted for, so the superclass still measures anything added outside `show()` and still recomputes the virtual size. An ASCII cell's display width is exactly its length, and both markup parsing and newline truncation can only shorten it, so `len(raw) <= width and raw.isascii()` proves a cell cannot widen its column without measuring it -- which settles almost every cell. Cold build, same fixture: 1,000 rows 142.5 ms -> 111.3 ms 10,000 rows 1010.4 ms -> 387.6 ms 50,000 rows 5017.1 ms -> 1507.0 ms (3.3x) Resulting widths are identical to the superclass result, pinned by a test that compares against a control DataTable seeded with the same cells, including CJK and markup-bearing values. Fixes #210 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Optimizes ResourceTable startup and rebuilds by reusing known cell widths instead of letting Textual remeasure every row.
Changes:
- Absorbs emitted-cell widths during rebuilds and appends.
- Filters already-measured rows from Textual’s dimension pass.
- Adds width-equivalence and fallback-path tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/korvid/ui/widgets/resource_table.py |
Implements column-width absorption. |
tests/ui/test_table_column_widths.py |
Tests performance invariant and width correctness. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review catch on #211: `_update_column_widths` drains queued cells *before* the dimension pass, and re-measures every cell in a column whenever the queued value looks narrower than the column. Absorbing a freshly appended row's widths synchronously could make that true for a cell queued earlier in the same repaint -- reintroducing the very O(total rows) measuring pass this change removes. Stop queuing entirely: every in-place cell update now passes update_width=False, and the widths of the rows that actually changed (appended or patched) are absorbed directly. `_absorb_widths` requests the dimension pass itself when a column genuinely grew, since update_width=False no longer schedules one. `_update_column_widths` can now never run from this widget, so the rescan is unreachable rather than merely unlikely. Extract `_patch_row` to keep `_apply_in_place` under the complexity gate. Test: `test_widening_new_row_does_not_trigger_a_full_column_rescan` spies on `get_column`, which only the rescan branch reaches. It fails before this commit (1 column rescanned) and passes after. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/korvid/ui/widgets/resource_table.py:628
- The required fail-safe for Textual's row-label/auto-height branches is missing: every emitted key is excluded without checking its
Rowmetadata. Issue #210 explicitly requires the fast path to be disabled if the current height-1/unlabelled invariant changes; otherwise a futureheight=Nonerow would keep height 0, or a labelled row would never update the label column. Only exclude rows that are confirmed height 1, non-auto-height, and unlabelled.
absorbed = self._emitted
super()._update_dimensions([key for key in new_rows if key.value not in absorbed])
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
After #208 a repaint costs only what changed, but the first build of a large
view still froze the UI for seconds — and so did every path that has to rebuild
rather than diff: switching kind, changing the sort, applying a custom view, and
the reorder fallback (a row inserted mid-table, since
add_rowcan only append).Splitting a 50,000-row cold build by wall clock showed the freeze is mostly not
our cell construction and not
add_row:show()itself (build cells +add_rowloop)That settling is
DataTable._update_dimensions(), which runs fromon_idleoverevery newly added row and, for each one, rebuilds the row's renderables through
default_cell_formatterand callsrich.measure()on every cell — 700,000renderable constructions and 700,000 measurements, to compute fourteen integers,
from cells we had just finished building.
Approach
ResourceTablealready knows every cell it emitted (_emitted, added in #208)and already has
_cell_width()— a cheap width function written to matchDataTable's own measurement, used today by the in-place diff._absorb_widths()folds the emitted cells straight intocolumn.content_width, on both seeding paths (the rebuild path, and the rowsthe in-place diff appends).
_update_dimensions()is overridden to handsuper()only the rows thatwere not absorbed. The superclass still measures anything added outside
show()— including a row added after the absorption but before the idle pass— and still recomputes
virtual_sizeexactly as before.string's display width is exactly its length, and both markup parsing and the
newline truncation
_cell_width()performs can only shorten it. Solen(raw) <= width and raw.isascii()proves the cell cannot widen its column,using two C-level calls and no measurement.
Failure mode if a future Textual renames the hook: the override simply stops
being called and the widget degrades to today's slower-but-correct measuring
pass.
Results
Cold
show()+ settle, same fixture, median of 3:The width absorption itself costs ~50 ms at 50,000 rows.
Tests
New
tests/ui/test_table_column_widths.py:test_seeding_does_not_rebuild_every_row_to_measure_it— the RED test.Without the change 400 of 400 rows are rebuilt for measurement; with it only
the handful painted on screen are.
test_absorbed_widths_match_what_textual_would_measure— compares theresulting
content_widthagainst a controlDataTableseeded with the samecells, over plain, long, full-width (CJK) and markup-bearing values. Identical.
test_rows_added_outside_show_are_still_measuredtest_unabsorbed_row_added_before_the_dimension_pass_is_measured— guards thecase where absorption and a direct
add_rowland in the same idle window.test_row_appearing_in_place_widens_its_column— a newly appeared long namestill grows the column through the in-place diff.
Verification
make checkon this branch: ruff ✅, ruff format ✅, mypy --strict ✅,4081 passed / 21 skipped, tach ✅.
Not in scope
DataTable.add_row's per-row overhead (measuredat ~2.8x on that phase, worth ~0.3 s of the remaining 1.5 s, but reaches much
deeper into Textual internals).
than removing it.
Fixes #210
Live-cluster validation
The numbers above are from synthetic summaries, which isolate the render path
but have uniform, all-ASCII cell content. To check the optimization holds on
real data, the same measurement was run against a live AKS cluster (v1.35.6)
holding 5,917 real pods across 6 namespaces, pulled through korvid's own
KubeClient— real manifests, real name/namespace length distribution, and twocustom columns (#45) sourced from annotations carrying Korean text, so the
non-ASCII path is genuinely exercised.
show()+ settle, beforeshow()+ settle, afterWidths came out byte-identical on both branches and matched the control
DataTable, including the two full-width columns (measured 30 and 45 cells wideagainst far fewer characters) — so the
isascii()fast path does not silentlyunder-measure CJK content.