-
Notifications
You must be signed in to change notification settings - Fork 11
perf: cache header/side offsets while computing cell geometry #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,6 +88,10 @@ export default class aTable extends aTemplate { | |
| this.convert = {}; | ||
| this.convert.getStyleByAlign = this.getStyleByAlign; | ||
| this.convert.setClass = this.setClass; | ||
| // getCellInfoByIndex が参照するヘッダー座標のスナップショット。 | ||
| // _withGeometryCache() のスコープ内だけ保持し、その外では常に null | ||
| this._geo = null; | ||
| this._geoDepth = 0; | ||
| const html = ` | ||
| <div class='a-table-container'> | ||
| <div data-id='${this.menu_id}'></div> | ||
|
|
@@ -207,6 +211,47 @@ export default class aTable extends aTemplate { | |
| return this._getElementByQuery(`[data-cell-id='${x}-${y}']`); | ||
| } | ||
|
|
||
| // getCellInfoByIndex はセル 1 個の論理座標を求めるたびに | ||
| // .js-table-header th (列数) と .js-table-side (行数) の座標を読み直すため、 | ||
| // 全セルを走査する処理では getBoundingClientRect が | ||
| // 「セル数 × (列数 + 行数)」回発生する。レイアウトが変化しない同期処理の | ||
| // 間だけ 1 回分をスナップショットして共有するためのキャッシュ | ||
| _buildGeometryCache() { | ||
| const headers = this._getElementsByQuery('.js-table-header th'); | ||
| const sides = this._getElementsByQuery('.js-table-side'); | ||
| return { | ||
| headerLefts: [].map.call(headers, header => util.offset(header).left), | ||
| sideTops: [].map.call(sides, side => util.offset(side).top) | ||
| }; | ||
| } | ||
|
|
||
| // スコープ外 (_geoDepth === 0) では保持せず毎回読み直すので、 | ||
| // ラップされていない経路の挙動・コストは変更前と同じ | ||
| _getGeometryCache() { | ||
| if (this._geo) { | ||
| return this._geo; | ||
| } | ||
| const geo = this._buildGeometryCache(); | ||
| if (this._geoDepth > 0) { | ||
| this._geo = geo; | ||
| } | ||
| return geo; | ||
| } | ||
|
|
||
| // fn の実行中だけスナップショットを共有する。入れ子で呼ばれた場合は | ||
| // 最も外側のスコープが閉じるまで保持し、閉じた時点で必ず破棄する | ||
| _withGeometryCache(fn) { | ||
| this._geoDepth += 1; | ||
| try { | ||
| return fn(); | ||
| } finally { | ||
| this._geoDepth -= 1; | ||
| if (this._geoDepth === 0) { | ||
| this._geo = null; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| getCellInfoByIndex(x, y) { | ||
| const cell = this.getCellByIndex(x, y); | ||
| if (!cell) { | ||
|
|
@@ -219,15 +264,14 @@ export default class aTable extends aTemplate { | |
| let returnTop = -1; | ||
| const width = parseInt(cell.getAttribute('colspan')); | ||
| const height = parseInt(cell.getAttribute('rowspan')); | ||
| const headers = this._getElementsByQuery('.js-table-header th'); | ||
| const sides = this._getElementsByQuery('.js-table-side'); | ||
| [].forEach.call(headers, (header, index) => { | ||
| if (util.offset(header).left === left) { | ||
| const geometry = this._getGeometryCache(); | ||
| geometry.headerLefts.forEach((headerLeft, index) => { | ||
| if (headerLeft === left) { | ||
| returnLeft = index; | ||
| } | ||
| }); | ||
| [].forEach.call(sides, (side, index) => { | ||
| if (util.offset(side).top === top) { | ||
| geometry.sideTops.forEach((sideTop, index) => { | ||
| if (sideTop === top) { | ||
| returnTop = index; | ||
| } | ||
| }); | ||
|
|
@@ -593,6 +637,8 @@ export default class aTable extends aTemplate { | |
| } | ||
|
|
||
| onUpdated() { | ||
| // 直前に update() が DOM を作り直しているのでスナップショットは無効 | ||
| this._geo = null; | ||
| const table = this._getElementByQuery('table'); | ||
| const inner = this._getSelf().parentNode; | ||
| const elem = this._getElementByQuery('.a-table-selected .a-table-editable'); | ||
|
|
@@ -611,6 +657,8 @@ export default class aTable extends aTemplate { | |
| } else { | ||
| inner.style.width = 'auto'; | ||
| } | ||
| // 上の幅の書き換えでレイアウトが動くため、ここでも破棄しておく | ||
| this._geo = null; | ||
|
|
||
| if (this.afterRendered) { | ||
| this.afterRendered(); | ||
|
|
@@ -1196,6 +1244,8 @@ export default class aTable extends aTemplate { | |
| } | ||
|
|
||
| beforeUpdated() { | ||
| // update() の呼び出し元が DOM を触っている可能性があるので破棄してから測り直す | ||
| this._geo = null; | ||
| this.changeSelectOption(); | ||
| this.markup(); | ||
| } | ||
|
|
@@ -1575,3 +1625,43 @@ export default class aTable extends aTemplate { | |
| } | ||
|
|
||
| } | ||
|
|
||
| // 1 回の同期処理の中で getCellInfoByIndex を複数回呼ぶメソッド。 | ||
| // 実行中だけヘッダー座標のスナップショットを共有させることで、 | ||
| // getBoundingClientRect の呼び出しを「セル数 × (列数 + 行数)」から | ||
| // 「列数 + 行数 + セル数」へ減らす。 | ||
| // ラップは前後で _geoDepth を増減するだけで、引数・戻り値・例外・ | ||
| // 副作用はいずれも元のメソッドのまま変わらない。 | ||
| // | ||
| // 一方、次のメソッドは意図的に対象外にしている。 | ||
| // - updateTable: イベントの振り分け役で、分岐の中で update() や | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drag-selection (mousemove) doesn't benefit from this cacheExcluding Since |
||
| // putCaret()、コピー / ペーストが走り DOM が変わるため | ||
| // - mergeCells: 座標を読んだあとに confirm() でブラウザへ制御を返すため | ||
| // (重い isSelectedCellsRectangle() 側でキャッシュは効く) | ||
| // - getClipBoardData / processPaste: DOM を直接書き換える、または | ||
| // setTimeout でペーストを待つ非同期経路のため | ||
| const GEOMETRY_SCOPED_METHODS = [ | ||
| 'getSelectedPoints', | ||
| 'getAllPoints', | ||
| 'getCellIndexByPos', | ||
| 'markup', | ||
| 'selectRange', | ||
| 'selectRow', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The stated criterion for this list doesn't match realityThe comment right above this list says the criterion is "methods that call 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 |
||
| 'selectCol', | ||
| 'removeRow', | ||
| 'removeCol', | ||
| 'insertRowAbove', | ||
| 'insertRowBelow', | ||
| 'insertColLeft', | ||
| 'insertColRight', | ||
| 'insertTable', | ||
| 'splitCell', | ||
| 'isSelectedCellsRectangle' | ||
| ]; | ||
|
|
||
| GEOMETRY_SCOPED_METHODS.forEach((name) => { | ||
| const method = aTable.prototype[name]; | ||
| aTable.prototype[name] = function geometryScoped(...args) { | ||
| return this._withGeometryCache(() => method.apply(this, args)); | ||
| }; | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,237 @@ | ||
| import { describe, it, expect, afterEach, vi } from 'vitest'; | ||
| import aTable from '../src/index.js'; | ||
| import { layoutSimpleTable, mockRect } from './helpers.js'; | ||
|
|
||
| function createTable(html) { | ||
| document.body.innerHTML = `<table class="table">${html}</table>`; | ||
| return new aTable('.table'); | ||
| } | ||
|
|
||
| const grid3x3 = '<tr><td>A</td><td>B</td><td>C</td></tr>' | ||
| + '<tr><td>D</td><td>E</td><td>F</td></tr>' | ||
| + '<tr><td>G</td><td>H</td><td>I</td></tr>'; | ||
|
|
||
| // _withGeometryCache を素通しにすると _geoDepth が 0 のままになり、 | ||
| // getCellInfoByIndex は毎回ヘッダー座標を読み直す (= キャッシュ導入前と同じ経路) | ||
| function withoutGeometryCache(instance, fn) { | ||
| const original = instance._withGeometryCache; | ||
| instance._withGeometryCache = f => f(); | ||
| try { | ||
| return fn(); | ||
| } finally { | ||
| instance._withGeometryCache = original; | ||
| } | ||
| } | ||
|
|
||
| // ヘッダー / サイドに対する getBoundingClientRect の呼び出し回数を数える。 | ||
| // layoutSimpleTable が差し替えたモックの戻り値はそのまま使う | ||
| function countGeometryReads(instance) { | ||
| const counter = { calls: 0 }; | ||
| const elements = [ | ||
| ...instance._getElementsByQuery('.js-table-header th'), | ||
| ...instance._getElementsByQuery('.js-table-side') | ||
| ]; | ||
| elements.forEach((element) => { | ||
| const original = element.getBoundingClientRect.bind(element); | ||
| element.getBoundingClientRect = () => { | ||
| counter.calls += 1; | ||
| return original(); | ||
| }; | ||
| }); | ||
| counter.size = elements.length; | ||
| return counter; | ||
| } | ||
|
|
||
| describe('座標キャッシュ: 結果が変わらないこと', () => { | ||
| afterEach(() => { | ||
| document.body.innerHTML = ''; | ||
| }); | ||
|
|
||
| it('getAllPoints: キャッシュの有無で同じ結果を返す', () => { | ||
| const t = createTable(grid3x3); | ||
| layoutSimpleTable(t, 3, 3); | ||
| const cached = t.getAllPoints(); | ||
| const uncached = withoutGeometryCache(t, () => t.getAllPoints()); | ||
| expect(cached).toHaveLength(9); | ||
| expect(cached).toEqual(uncached); | ||
| }); | ||
|
|
||
| it('getAllPoints: 結合セルを含む表でもキャッシュの有無で同じ結果を返す', () => { | ||
| const t = createTable('<tr><td colspan="2">A</td><td>C</td></tr>' | ||
| + '<tr><td>D</td><td>E</td><td>F</td></tr>'); | ||
| layoutSimpleTable(t, 3, 2); | ||
| // C は colspan=2 の A の右隣なので論理 x は 2 (left = 300) | ||
| mockRect(t.getCellByIndex(1, 0), { left: 300, top: 0 }); | ||
| const cached = t.getAllPoints(); | ||
| const uncached = withoutGeometryCache(t, () => t.getAllPoints()); | ||
| expect(cached).toEqual([ | ||
| { x: 0, y: 0, width: 2, height: 1 }, | ||
| { x: 2, y: 0, width: 1, height: 1 }, | ||
| { x: 0, y: 1, width: 1, height: 1 }, | ||
| { x: 1, y: 1, width: 1, height: 1 }, | ||
| { x: 2, y: 1, width: 1, height: 1 } | ||
| ]); | ||
| expect(cached).toEqual(uncached); | ||
| }); | ||
|
|
||
| it('getCellIndexByPos: キャッシュの有無で同じ結果を返す', () => { | ||
| const t = createTable(grid3x3); | ||
| layoutSimpleTable(t, 3, 3); | ||
| for (let y = 0; y < 3; y += 1) { | ||
| for (let x = 0; x < 3; x += 1) { | ||
| const cached = t.getCellIndexByPos(x, y); | ||
| const uncached = withoutGeometryCache(t, () => t.getCellIndexByPos(x, y)); | ||
| expect(cached).toEqual({ row: y, col: x }); | ||
| expect(cached).toEqual(uncached); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| it('一連の編集操作の結果 (getTable) がキャッシュの有無で一致する', () => { | ||
| const scenario = (disableCache) => { | ||
| const t = createTable(grid3x3); | ||
| if (disableCache) { | ||
| t._withGeometryCache = f => f(); | ||
| } | ||
| layoutSimpleTable(t, 3, 3); | ||
| t.select(0, 0); | ||
| t.selectRange(1, 1); | ||
| layoutSimpleTable(t, 3, 3); | ||
| t.insertColRight(1); | ||
| layoutSimpleTable(t, 4, 3); | ||
| t.insertRowBelow(0); | ||
| layoutSimpleTable(t, 4, 4); | ||
| // selectRow は contextmenu() 経由でイベントを参照する | ||
| t.e = { preventDefault: () => {}, clientX: 0, clientY: 0 }; | ||
| t.selectRow(2); | ||
| layoutSimpleTable(t, 4, 4); | ||
| t.removeCol(3); | ||
| const html = t.getTable(); | ||
| document.body.innerHTML = ''; | ||
| return html; | ||
| }; | ||
| const withCache = scenario(false); | ||
| const withoutCache = scenario(true); | ||
| expect(withCache).toBe(withoutCache); | ||
| expect(withCache).toContain('<td>A</td>'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('座標キャッシュ: 計測回数', () => { | ||
| afterEach(() => { | ||
| document.body.innerHTML = ''; | ||
| }); | ||
|
|
||
| it('getAllPoints 中のヘッダー座標の計測は 1 巡だけになる', () => { | ||
| const t = createTable(grid3x3); | ||
| layoutSimpleTable(t, 3, 3); | ||
| const counter = countGeometryReads(t); | ||
|
|
||
| t.getAllPoints(); | ||
| const cachedCalls = counter.calls; | ||
|
|
||
| counter.calls = 0; | ||
| withoutGeometryCache(t, () => t.getAllPoints()); | ||
| const uncachedCalls = counter.calls; | ||
|
|
||
| // キャッシュあり: ヘッダー + サイドを 1 回ずつ | ||
| expect(cachedCalls).toBe(counter.size); | ||
| // キャッシュなし: セル 9 個ぶん繰り返す | ||
| expect(uncachedCalls).toBe(counter.size * 9); | ||
| expect(cachedCalls).toBeLessThan(uncachedCalls); | ||
| }); | ||
|
|
||
| it('入れ子で呼ばれてもスナップショットは 1 回しか作られない', () => { | ||
| const t = createTable(grid3x3); | ||
| layoutSimpleTable(t, 3, 3); | ||
| const counter = countGeometryReads(t); | ||
| // isSelectedCellsRectangle は getSelectedPoints / getAllPoints / getCellByPos を | ||
| // まとめて呼ぶので、入れ子のスコープでキャッシュが共有されることを確認する | ||
| t.select(0, 0); | ||
| counter.calls = 0; | ||
| t.isSelectedCellsRectangle(); | ||
| expect(counter.calls).toBe(counter.size); | ||
| }); | ||
| }); | ||
|
|
||
| describe('座標キャッシュ: 破棄のタイミング', () => { | ||
| afterEach(() => { | ||
| document.body.innerHTML = ''; | ||
| }); | ||
|
|
||
| it('スコープを抜けたらキャッシュは破棄される', () => { | ||
| const t = createTable(grid3x3); | ||
| layoutSimpleTable(t, 3, 3); | ||
| t.getAllPoints(); | ||
| expect(t._geo).toBeNull(); | ||
| expect(t._geoDepth).toBe(0); | ||
| }); | ||
|
|
||
| it('スコープ内ではキャッシュを保持し、最も外側を抜けたときに破棄する', () => { | ||
| const t = createTable(grid3x3); | ||
| layoutSimpleTable(t, 3, 3); | ||
| let geoInScope = null; | ||
| let depthInScope = -1; | ||
| t._withGeometryCache(() => { | ||
| t.getAllPoints(); | ||
| geoInScope = t._geo; | ||
| depthInScope = t._geoDepth; | ||
| }); | ||
| expect(depthInScope).toBe(1); | ||
| expect(geoInScope).not.toBeNull(); | ||
| expect(geoInScope.headerLefts).toEqual([0, 100, 200, 300]); | ||
| expect(geoInScope.sideTops).toEqual([0, 50, 100]); | ||
| expect(t._geo).toBeNull(); | ||
| }); | ||
|
|
||
| it('例外が投げられてもキャッシュとスコープの深さは元に戻る', () => { | ||
| const t = createTable(grid3x3); | ||
| layoutSimpleTable(t, 3, 3); | ||
| expect(() => { | ||
| t._withGeometryCache(() => { | ||
| t.getAllPoints(); | ||
| throw new Error('boom'); | ||
| }); | ||
| }).toThrow('boom'); | ||
| expect(t._geo).toBeNull(); | ||
| expect(t._geoDepth).toBe(0); | ||
| }); | ||
|
|
||
| it('update() は DOM を作り直すのでキャッシュを破棄する', () => { | ||
| const t = createTable(grid3x3); | ||
| layoutSimpleTable(t, 3, 3); | ||
| t.update(); | ||
| expect(t._geo).toBeNull(); | ||
|
|
||
| // スコープの内側で update() が走った場合も破棄され、以降は測り直す | ||
| const build = vi.spyOn(t, '_buildGeometryCache'); | ||
| t._withGeometryCache(() => { | ||
| t.getAllPoints(); | ||
| expect(t._geo).not.toBeNull(); | ||
| const buildsBeforeUpdate = build.mock.calls.length; | ||
| t.update(); | ||
| expect(t._geo).toBeNull(); | ||
| t.getAllPoints(); | ||
| expect(build.mock.calls.length).toBeGreaterThan(buildsBeforeUpdate); | ||
| }); | ||
| build.mockRestore(); | ||
| }); | ||
|
|
||
| it('呼び出しをまたいでキャッシュが残らず、レイアウト変更後は測り直す', () => { | ||
| const t = createTable(grid3x3); | ||
| layoutSimpleTable(t, 3, 3); | ||
| expect(t.getCellInfoByIndex(1, 1)).toEqual({ x: 1, y: 1, width: 1, height: 1 }); | ||
|
|
||
| // 行の高さが変わった状況 (top が 50 刻みから 80 刻みへ) を模す | ||
| const sides = t._getElementsByQuery('.js-table-side'); | ||
| [].forEach.call(sides, (side, index) => { | ||
| mockRect(side, { top: index * 80 }); | ||
| }); | ||
| for (let y = 0; y < 3; y += 1) { | ||
| for (let x = 0; x < 3; x += 1) { | ||
| mockRect(t.getCellByIndex(x, y), { left: (x + 1) * 100, top: y * 80 }); | ||
| } | ||
| } | ||
| expect(t.getCellInfoByIndex(1, 1)).toEqual({ x: 1, y: 1, width: 1, height: 1 }); | ||
| }); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this._geo = nullinvalidation is copy-pasted in 3 placesThe same "discard the geometry cache" operation appears in two places inside
onUpdated()(here and line 661) and once insidebeforeUpdated()(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.