Skip to content

test(grid): add grid scroll performance benchmarks - #2746

Open
mofojed wants to merge 15 commits into
deephaven:mainfrom
mofojed:grid-perf-benchmark-tooling
Open

test(grid): add grid scroll performance benchmarks#2746
mofojed wants to merge 15 commits into
deephaven:mainfrom
mofojed:grid-perf-benchmark-tooling

Conversation

@mofojed

@mofojed mofojed commented Aug 28, 2026

Copy link
Copy Markdown
Member
  • Adds two Playwright benchmark suites, both skipped unless RUN_PERF_TESTS is set since frame timings are too resource sensitive for CI.
  • grid-performance.spec.ts measures scroll FPS in the main app against real tables from a Deephaven server.
  • grid-perf-app.spec.ts drives a standalone Vite app backed by MockGridModel, so the grid can be benchmarked without a server and at row and column counts the test data does not reach.
  • Will be used as a baseline to measure performance of Grid changes

Adds two Playwright benchmark suites, both skipped unless RUN_PERF_TESTS
is set since frame timings are too resource sensitive for CI.

grid-performance.spec.ts measures scroll FPS in the main app against real
tables from a Deephaven server.

grid-perf-app.spec.ts drives a standalone Vite app backed by MockGridModel,
so the grid can be benchmarked without a server and at row and column counts
the test data does not reach.
@mofojed
mofojed requested a balanced review from Copilot August 28, 2026 17:53
@mofojed mofojed self-assigned this Aug 28, 2026
@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.02%. Comparing base (535936a) to head (dfc3135).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2746      +/-   ##
==========================================
+ Coverage   51.80%   52.02%   +0.22%     
==========================================
  Files         808      808              
  Lines       46321    46329       +8     
  Branches    11849    11850       +1     
==========================================
+ Hits        23996    24104     +108     
+ Misses      22305    22205     -100     
  Partials       20       20              
Flag Coverage Δ
unit 52.02% <ø> (+0.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

Pull request overview

Adds opt-in Playwright benchmarks for Grid scrolling performance in the main application and a standalone mock-data app.

Changes:

  • Adds real-table and standalone Grid FPS benchmarks.
  • Adds a configurable Vite performance app using MockGridModel.
  • Documents and exposes benchmark commands.

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/grid-performance.spec.ts Adds main-app scroll benchmarks.
tests/grid-perf-app.spec.ts Adds standalone Grid benchmarks.
tests/grid-perf-app/src/App.tsx Renders a configurable mock Grid.
tests/grid-perf-app/src/main.tsx Initializes the React app.
tests/grid-perf-app/vite.config.ts Configures the performance server.
tests/grid-perf-app/tsconfig.json Configures TypeScript.
tests/grid-perf-app/package.json Defines app dependencies and scripts.
tests/grid-perf-app/package-lock.json Locks app dependencies.
tests/grid-perf-app/index.html Provides the app shell and sizing.
tests/grid-perf-app/.gitignore Excludes generated artifacts.
README.md Documents benchmark workflows.
package.json Adds benchmark commands.
Files not reviewed (1)
  • tests/grid-perf-app/package-lock.json: Generated file
Suppressed comments (5)

tests/grid-performance.spec.ts:181

  • all_types is created with only 20 rows (tests/docker-scripts/data/app.d/common_tables.py:4,27), roughly 380px at the Iris Grid row height, so it has no vertical overflow in the normal E2E viewport. Both tests in this block send only vertical wheel deltas, meaning the Grid view does not change and the measured FPS is effectively idle-page FPS. Use a fixture with enough rows or scroll the overflowing column axis.
      // all_types is a table with many different column types
      await openTable(page, 'all_types');

tests/grid-performance.spec.ts:203

  • This benchmark never positions the mouse over the Grid. openTable leaves it at the Panels menu click, while Grid registers its non-passive wheel handler directly on the canvas, so these events can target unrelated page content and measure no Grid work. Hover the canvas before starting the measurement.
      for (let i = 0; i < 50; i += 1) {
        await page.mouse.wheel(0, 200);

tests/grid-performance.spec.ts:239

  • The mouse is still at the control used by openTable, not explicitly over the Grid canvas. Because the wheel listener is attached directly to the canvas, this loop can run for three seconds without scrolling or redrawing the Grid. Hover the canvas before starting timing.
    while (Date.now() - startTime < duration) {
      await page.mouse.wheel(0, 300 * direction);

tests/grid-performance.spec.ts:244

  • Using Math.random() changes the scroll-direction sequence and how long the grid sits at an edge on every run, adding avoidable variance to a performance baseline. Reverse at a deterministic event interval (or use a seeded sequence) so results from two revisions represent the same workload.
      // Reverse direction occasionally
      if (Math.random() < 0.1) {
        direction *= -1;

tests/grid-perf-app.spec.ts:196

  • Using Math.random() gives each benchmark run a different workload and can change how much time is spent at the top boundary, adding avoidable variance to the baseline. Reverse at a deterministic event interval (or use a seeded sequence) so revisions are compared with identical input.
      if (Math.random() < 0.1) {
        direction *= -1;
      }

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/grid-perf-app/vite.config.ts
Comment thread tests/grid-performance.spec.ts Outdated
Comment thread tests/grid-perf-app.spec.ts Outdated
Comment thread package.json Outdated
Adds two Playwright benchmark suites, both skipped unless RUN_PERF_TESTS
is set since frame timings are too resource sensitive for CI.

grid-performance.spec.ts measures scroll FPS in the main app against real
tables from a Deephaven server.

grid-perf-app.spec.ts drives a standalone Vite app backed by MockGridModel,
so the grid can be benchmarked without a server and at row and column counts
the test data does not reach.
Copilot AI review requested due to automatic review settings August 31, 2026 20:10

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.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • tests/grid-perf-app/package-lock.json: Generated file
Suppressed comments (1)

tests/grid-performance.spec.ts:257

  • The random reversals make this baseline non-repeatable. Because simple_table has only 100 rows, one random sequence may spend much of the three seconds pinned at an edge (where wheel events do not redraw), while another keeps moving; the resulting FPS difference can come from the workload rather than a Grid change. Use a fixed or seeded reversal schedule.
      // Reverse direction occasionally
      if (Math.random() < 0.1) {
        direction *= -1;
      }

Comment thread tests/grid-performance.spec.ts Outdated
- Add a big all_types table that can be scrolled
Copilot AI review requested due to automatic review settings August 31, 2026 20:42

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.

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • tests/grid-perf-app/package-lock.json: Generated file
Suppressed comments (6)

Previously missed (4) — in code that hasn't changed since the last review.

tests/grid-performance.spec.ts:168

  • Browser-side failures are not observed here, so a grid exception or console error can leave the page's requestAnimationFrame loop running and produce a successful-looking FPS result. Install the same error listeners used by tests/table-scroll.spec.ts:46-54 before navigating so a broken benchmark fails instead of reporting idle-page FPS.
  test.beforeEach(async ({ page }) => {
    await gotoPage(page, '');
  });

tests/grid-perf-app.spec.ts:139

  • Browser errors do not automatically fail Playwright tests, so this benchmark can keep measuring the page's animation loop after the standalone Grid has crashed. Add error listeners before each test so logged timings are only accepted from a functioning grid.

This issue also appears on line 194 of the same file.

  test.describe.configure({ mode: 'serial' });

tests/grid-performance.spec.ts:213

  • These wheel events are sent at the mouse position left by openTable (the Panels menu item), unlike scrollGrid, which explicitly moves to the grid. Depending on what occupies that coordinate after the menu closes, this test can measure an idle page without scrolling the Grid; hover the target grid before starting the measurement.

This issue also appears on line 253 of the same file.

        await page.mouse.wheel(0, 200);

tests/grid-performance.spec.ts:260

  • Using random direction changes makes the amount and location of scrolling different on every run, so before/after FPS results do not use a repeatable workload and cannot serve as a reliable baseline. Reverse after a fixed number of wheel events instead.
      if (Math.random() < 0.1) {

tests/grid-performance.spec.ts:253

  • This test also sends wheel events without first targeting the newly opened grid, so the events remain at the Panels menu click coordinate and may not reach the canvas wheel listener. Hover the grid before measuring to ensure this is sustained Grid scrolling rather than idle-page FPS.
      await page.mouse.wheel(0, 300 * direction);

tests/grid-perf-app.spec.ts:194

  • Random direction changes make each benchmark run execute a different scroll path, which adds workload variance to the FPS comparison. Use a fixed reversal cadence so this suite provides a reproducible baseline.
      if (Math.random() < 0.1) {

Comment thread tests/docker-scripts/data/app.d/common_tables.py Outdated
- The Byte column broke after scrolling down
- mod the input to be within bounds for valid Bytes
Copilot AI review requested due to automatic review settings September 1, 2026 14:14

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.

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • tests/grid-perf-app/package-lock.json: Generated file
Suppressed comments (6)

Previously missed (6) — in code that hasn't changed since the last review.

tests/grid-perf-app.spec.ts:130

  • minFps is only printed, so every standalone benchmark passes regardless of measured performance. Enforce the supplied minimum so regressions produce a failing result.
  console.log(`  Expected min FPS: ${expected.minFps}`);

tests/grid-perf-app.spec.ts:197

  • Using Math.random() changes the scroll path on every run and can pin the grid at the top when an upward segment exceeds prior downward movement, so results are not comparable as a performance baseline. Use a deterministic down/up workload.
      if (Math.random() < 0.1) {

tests/grid-performance.spec.ts:147

  • minFps is only printed, so these benchmarks still pass when FPS is below the stated minimum (even at 0 FPS). Enforce the threshold so the command can detect regressions rather than always exiting successfully.
  console.log(`  Expected min FPS: ${expected.minFps}`);

tests/grid-performance.spec.ts:213

  • These wheel events are sent at the mouse's current position, which openTable leaves on the Panels menu item rather than explicitly over the grid. If that coordinate is outside the new canvas, this benchmark measures idle animation frames. Position the pointer over the grid before starting the measurement, as the other scroll tests do.
      await startFPSMeasurement(page);

tests/grid-performance.spec.ts:260

  • Random reversals make the workload non-repeatable and, with only 100 rows, can leave the grid pinned at an edge while idle frames inflate the reported FPS. Use a deterministic down/up pattern that keeps actual scrolling active throughout the measurement.
      if (Math.random() < 0.1) {

tests/grid-perf-app/README.md:26

  • The documented benchmark runs Vite/React in development mode; combined with the app's StrictMode, React performs development-only extra renders, so the captured timings do not represent production Grid performance. Build and serve the production bundle for benchmark runs (the existing preview config already uses port 4020).
npm run dev

Move grid-performance.spec.ts, grid-perf-app.spec.ts, and the standalone perf
app into tests/grid-perf so it is clear they only run as part of the perf
suites. The Vite app is nested at tests/grid-perf/app, keeping the specs
outside its "type": "module" scope.
Adds console-generated tables of ~500 and ~25,000 character string cells to exercise text measurement and truncation during scrolling.
Copilot AI review requested due to automatic review settings September 2, 2026 13:59

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.

🟡 Changes recommended

Several benchmarks can measure an idle grid, use nondeterministic workloads, or pass despite missing or sub-threshold measurements.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (1)

  • tests/grid-perf/app/package-lock.json: Generated file

Suppressed comments (1)

tests/grid-perf/grid-performance.spec.ts:316

  • The pointer remains at the table entry clicked by openTable, so these wheel events are not deterministically targeted at the grid. This can produce an apparently healthy FPS result while the grid remains idle; hover the grid before measuring, as the other direct-wheel benchmarks do.
    await startFPSMeasurement(page);
  • Files reviewed: 15/16 changed files
  • Comments generated: 5
  • Review effort level: Balanced

Comment thread tests/grid-perf/grid-perf-app.spec.ts
Comment thread tests/grid-perf/grid-performance.spec.ts Outdated
Comment thread tests/grid-perf/grid-performance.spec.ts
Comment thread tests/grid-perf/grid-performance.spec.ts
Comment thread tests/grid-perf/utils.ts
Copilot AI review requested due to automatic review settings September 2, 2026 18:42

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.

🟡 Changes recommended

Three benchmark files fail linting because intentional sequential awaits lack scoped no-await-in-loop suppressions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (1)

  • tests/grid-perf/app/package-lock.json: Generated file

Suppressed comments (1)

tests/grid-perf/grid-performance.spec.ts:102

  • expect is not imported from @playwright/test, so this path throws ReferenceError: expect is not defined as soon as a long-string table opens. Import Playwright's expect, or wait on the locator directly.
  await expect(page.locator('.iris-grid .grid-wrapper')).toBeVisible();
  • Files reviewed: 15/16 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread tests/grid-perf/grid-perf-app.spec.ts
Comment thread tests/grid-perf/grid-performance.spec.ts
Comment thread tests/grid-perf/utils.ts Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 3, 2026 15:28

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.

🟡 Changes recommended

A missing import breaks several benchmarks, and the sustained benchmark may measure idle frames.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (1)

  • tests/grid-perf/app/package-lock.json: Generated file

Suppressed comments (1)

tests/grid-perf/grid-performance.spec.ts:318

  • This “sustained” benchmark uses a 100-row table, so 300-pixel wheel events reach an edge after only a few iterations. Grid.handleWheel performs no state update/redraw at an edge, meaning much of the three-second sample can measure idle requestAnimationFrame callbacks rather than sustained grid rendering. Use the million-row table already added by this PR so the workload remains active.
    await openTable(page, 'simple_table');
  • Files reviewed: 15/16 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread tests/grid-perf/grid-performance.spec.ts
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 3, 2026 15:36

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.

🟡 Changes recommended

The horizontal benchmarks currently measure an idle grid rather than actual scrolling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (1)

  • tests/grid-perf/app/package-lock.json: Generated file

Suppressed comments (1)

tests/grid-perf/grid-performance.spec.ts:272

  • As in the long-string horizontal case above, this table has only five default-width columns (about 500px total), so it fits the default desktop viewport and these horizontal wheel events do not move the grid. This reports idle-frame timings for the huge-string case. Force horizontal overflow and confirm that scrolling changes the grid position before measuring.
      for (let i = 0; i < 20; i += 1) {
        await page.mouse.wheel(600, 0);
        await page.waitForTimeout(16);
  • Files reviewed: 15/16 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment on lines +211 to +215
// Long string columns are wide, so horizontal scrolling changes which
// part of each cell is truncated on every frame
for (let i = 0; i < 20; i += 1) {
await page.mouse.wheel(600, 0);
await page.waitForTimeout(16);
The generated long string tables only had five columns, which fit within the viewport, so the horizontal benchmarks measured an idle grid. Add wide filler columns and verify the grid actually scrolls the benchmark distance before recording FPS.
Copilot AI review requested due to automatic review settings September 3, 2026 20:51

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.

🔵 Needs a closer look

CI fixture validation, video-free measurements, and valid vertical scroll ranges must be addressed.

Review details

Files not reviewed (1)

  • tests/grid-perf/app/package-lock.json: Generated file

Suppressed comments (4)

Previously missed (4) — in code that hasn't changed since the last review.

tests/grid-perf/app/package.json:8

  • The standalone app is outside the root workspace and root TypeScript references, while the repository lint configuration only matches packages/*/src; because its Playwright suite is skipped by default, CI never builds or type-checks this fixture. An import, SCSS, or type regression can therefore land unnoticed and make the benchmark unusable. Add a lightweight non-timing validation that installs and builds/type-checks this app in CI (or integrate it into the workspace validation) while leaving FPS execution opt-in.
    tests/grid-perf/grid-perf-app.spec.ts:29
  • This suite also inherits local video recording from playwright.config.ts:62, so its measured frame times include screen-capture overhead. Disable video for the performance describe block to keep standalone-grid baselines comparable.
    tests/grid-perf/grid-performance.spec.ts:133
  • The repository's Playwright config records video for every local test (video: 'retain-on-failure' at playwright.config.ts:62). That capture work runs during these FPS measurements and can materially lower or destabilize the baseline; override video to off for this performance suite so the benchmark measures grid rendering rather than recording overhead.
    tests/grid-perf/grid-performance.spec.ts:161
  • simple_table has 100 rows at the Grid's default 20 px row height, so its maximum vertical offset is strictly less than 2,000 px after subtracting the visible viewport. These first two 2,000 px legs therefore contain guaranteed clamped wheel steps, and the reported FPS includes idle frames rather than scroll rendering. Bound each leg to a verified available vertical range (as the horizontal benchmarks do), or use a larger table for this sequence.
  • Files reviewed: 15/16 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Adds a performance-tests job that runs the grid scroll benchmarks in chromium once the e2e matrix finishes, and aggregates the frame timings into a markdown report published to the job summary, an artifact, and a PR comment. The job is informational only since runner timings are noisy.
Adds performance_tables.py with perf_ prefixed tables for the grid scroll
benchmarks so the long string tables no longer have to be built through the
console, and moves the perf only all_types_big table out of common_tables.py.
Video and trace capture compete with rendering and skew the measured frame times, so the perf suites now run through playwright-perf.config.ts with capture off, chromium only, and a single worker.
Copilot AI review requested due to automatic review settings September 4, 2026 14:09

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.

🟡 Changes recommended

Critical lint and CI output-permission failures, plus benchmark and reporting correctness issues, remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (1)

  • tests/grid-perf/app/package-lock.json: Generated file

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

tests/grid-perf/grid-performance.spec.ts:75

  • simple_table has only 100 rows, so its total row content is about 2,000px and its actual scroll range is smaller than that after subtracting the viewport. These 2,000px legs therefore clamp at the edge, while scrollGrid continues emitting wheel events and recording idle frames; that inflates the FPS baseline. Keep every leg within the known scroll range (or verify continued movement while measuring).
  • Files reviewed: 20/21 changed files
  • Comments generated: 4
  • Review effort level: Balanced

Comment thread .github/workflows/e2e.yml
Comment on lines +209 to +210
- name: Run performance tests
run: './tests/docker-scripts/run.sh e2e-performance'
Comment thread tests/grid-perf/utils.ts
@@ -0,0 +1,251 @@
/* eslint-disable no-await-in-loop -- benchmark input must be sequential */
Comment thread .github/workflows/e2e.yml
Comment on lines +212 to +214
- name: Build report
if: ${{ !cancelled() }}
run: node scripts/grid-perf-report.mjs
}

const result = await stopFPSMeasurement(page);
logResults('Sustained Scroll (3s)', result, { minFps: 30 });
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants