perf: cache header/side offsets while computing cell geometry - #12
perf: cache header/side offsets while computing cell geometry#12sixbird wants to merge 1 commit into
Conversation
getCellInfoByIndex() re-measured every `.js-table-header th` (one per column) and every `.js-table-side` (one per row) each time a single cell's logical (x, y) was resolved. The methods that walk the whole table - markup(), getAllPoints(), getSelectedPoints(), getCellIndexByPos() - therefore issued `cells x (cols + rows)` getBoundingClientRect() calls, which made cell selection, drag selection, merge/split, row/column insertion and paste quadratic in table size and froze the browser on large tables. Take one snapshot of the header/side offsets and share it for the duration of a single synchronous coordinate-reading call, then drop it. The snapshot only exists inside _withGeometryCache() scopes and is discarded at every DOM-rewriting boundary (beforeUpdated(), and both before and after onUpdated() rewrites the wrapper width), so no measurement can outlive the layout it was taken from. Outside a scope the offsets are read exactly as before. Methods that are not wrapped on purpose: updateTable() (event dispatcher whose branches re-render), mergeCells() (hands control back to the browser through confirm(); its heavy isSelectedCellsRectangle() call is cached anyway), getClipBoardData()/processPaste() (mutate the DOM directly / wait for the paste asynchronously). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
uidev1116
left a comment
There was a problem hiding this comment.
パフォーマンス改善の方向性自体は良いと思います。細かい点をいくつかコメントしました。
| // 副作用はいずれも元のメソッドのまま変わらない。 | ||
| // | ||
| // 一方、次のメソッドは意図的に対象外にしている。 | ||
| // - updateTable: イベントの振り分け役で、分岐の中で update() や |
There was a problem hiding this comment.
Drag-selection (mousemove) doesn't benefit from this cache
Excluding updateTable() from the cached-method list makes sense on its own, but inside it, getSelectedPoints() (around line 846) is called unconditionally for every event type, and the mousemove branch (around line 885, selectRange()) opens a separate scope and recomputes the full geometry again.
Since points is only actually used in the mousedown / mouseup / touchstart branches, moving the getSelectedPoints() call inside those three branches should eliminate the double computation during dragging (where mousemove fires very frequently). Wrapping the whole function in the cache scope shouldn't be necessary.
| 'getCellIndexByPos', | ||
| 'markup', | ||
| 'selectRange', | ||
| 'selectRow', |
There was a problem hiding this comment.
The stated criterion for this list doesn't match reality
The comment right above this list says the criterion is "methods that call getCellInfoByIndex multiple times directly," but selectRow never calls getCellInfoByIndex directly. It calls getAllPoints() once, then loops calling getCellByPos → getCellIndexByPos (which internally scans every cell) multiple times — that's the actual reason it needs to share the cache scope.
This mismatch makes it easy for someone adding a similar method later (one that loops calling an already-wrapped helper multiple times) to misjudge it as "not applicable" just by reading the comment literally. Either rewrite the comment to match the real criterion, or consider flipping this from an allowlist to a denylist (exclude only updateTable / mergeCells / getClipBoardData / processPaste, and wrap everything else by default) to structurally prevent this kind of omission.
|
|
||
| onUpdated() { | ||
| // 直前に update() が DOM を作り直しているのでスナップショットは無効 | ||
| this._geo = null; |
There was a problem hiding this comment.
this._geo = null invalidation is copy-pasted in 3 places
The same "discard the geometry cache" operation appears in two places inside onUpdated() (here and line 661) and once inside beforeUpdated() (line 1248), each with its own explanatory comment.
Since this PR already introduces named APIs (_buildGeometryCache / _getGeometryCache / _withGeometryCache), extracting a single _invalidateGeometryCache() method and calling it from all three places would reduce the risk of missing one spot if the cache's internal structure changes later.
Problem
getCellInfoByIndex(x, y)resolves the logical coordinates of one cell, but to do so it re-measures every.js-table-header th(one per column) and every.js-table-side(one per row):Every method that walks the whole table calls it once per cell, so a full scan costs
cells x (cols + rows)getBoundingClientRect()calls, and each of those forces a layout flush. The scan happens on the hot paths:update()->beforeUpdated()->markup()andonUpdated()->getSelectedPoints()— i.e. every single click on a cellselectRange()— on everymousemovewhile drag-selectingmergeCells()/splitCell()/insertRow*()/insertCol*()/removeRow()/removeCol()/insertTable()(paste), several of which callgetCellIndexByPos()per point, making themO(rows x cells^2)On a 100 x 6 table this is ~65,000
getBoundingClientRect()calls for a single selection change, and the browser visibly freezes.Fix
While a coordinate-reading method is running, take one snapshot of the header/side offsets and share it:
_buildGeometryCache()returns{ headerLefts, sideTops }_getGeometryCache()returns the snapshot when one is open, otherwise measures and returns a fresh one without storing it — so any path that is not wrapped behaves and costs exactly as before_withGeometryCache(fn)opens a scope (_geoDepth++/finally->--_geoDepth === 0-> drop). Nested calls share the outermost snapshotWhy the output cannot change
A snapshot is only ever used inside a single synchronous call, and it is dropped at every point where the DOM/layout is rewritten:
beforeUpdated()drops it beforemarkup()re-measuresonUpdated()drops it on entry (the table has just been re-rendered byupdate()) and again at the end, becauseonUpdated()itself rewritesinner.style.width(9999px->tableWidth) and thereby moves the layoutfinally, so exceptions and thealert()/early-returnpaths are covered)So a measurement can never outlive the layout it was taken from; the resolved
(x, y)for any cell is identical to what the current code computes.Deliberately not wrapped
updateTable()update(),putCaret(), copy/paste, so the DOM changes inside it. Its inner calls are wrapped individually.mergeCells()confirm(), then reads again. Its expensiveisSelectedCellsRectangle()call is wrapped, so it keeps essentially all of the win.getClipBoardData()/processPaste()setTimeout.getSelectedPoint(),getCellByPos(),copyTable()Measurements
Deterministic call counts, measured against this patch in the vitest/jsdom setup (12 rows x 6 columns, 72 cells,
getAllPoints()):getBoundingClientRect()callsThe counts are
cells x (cols + rows + 1)vscols + rows + cells, i.e. for 100 x 6 it goes from ~64,800 to ~707.Wall-clock, measured in headless Chrome 151 on a 100 x 6 table with the same technique applied to the bundled 1.5.12 build (identical code structure to 1.6.0):
mousemovewhile drag-selectingTests
npm run prepack(lint + tests + build) passes: 187 tests, 10 files — the 176 existing tests are untouched and green, plus 11 new ones intest/geometry-cache.test.js:getAllPoints()(plain grid and acolspangrid),getCellIndexByPos()for every cell, and thegetTable()output after a sequence ofselectRange->insertColRight->insertRowBelow->selectRow->removeColgetAllPoints()on a 3 x 3 grid drops from7 x 9header/side reads to7; nested calls (isSelectedCellsRectangle()) build the snapshot only onceupdate()(includingupdate()called from inside an open scope, after which the next read re-measures), and it never leaks across calls — re-mocking the row layout between twogetCellInfoByIndex()calls yields the new coordinates概要 (日本語)
getCellInfoByIndex(x, y)はセル 1 個の論理座標を求めるたびに.js-table-header th(列数)と.js-table-side(行数)の座標を測り直しているため、全セルを走査する処理はセル数 x (列数 + 行数)回のgetBoundingClientRect()を発行します。これはセル選択(update()->markup()/getAllPoints())、ドラッグ選択(mousemoveごとのselectRange())、結合・分割・行列追加・貼り付け(insertTable())といった主要な操作すべてが通る経路で、100 行 x 6 列では 1 クリックあたり約 65,000 回に達し、ブラウザが固まります。対策として、座標を読むメソッドの実行中だけヘッダー / サイドの座標を 1 回スナップショットして共有します。スナップショットは
_withGeometryCache()のスコープ内にしか存在せず、DOM が書き換わる境界(beforeUpdated()、onUpdated()の冒頭と末尾)で必ず破棄するため、測った時点のレイアウトより長く生き残ることはなく、出力される HTML は変わりません。confirm()でブラウザに制御を返すmergeCells()、DOM を直接触るupdateTable()/getClipBoardData()/processPaste()は意図的に対象外にしています(重い内側の呼び出しはキャッシュされます)。既存の 176 テストはそのまま通り、キャッシュ有無での結果一致・計測回数の削減・破棄タイミングを確認する 11 件を追加しています。