test(shared-tree): Add execution time performance benchmarks for SharedTree table APIs #24881
Conversation
github-actions
bot
added
base: main
There was a problem hiding this comment.
Pull Request Overview
This PR introduces performance benchmark tests for SharedTree's table-related APIs that measure execution times for various table operations such as insertion and removal.
- Added benchmark tests for column, row, and combined insertion and removal operations.
- Updated Mocha configuration to filter performance-relevant tests and included helper functions for table tree initialization.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/dds/tree/src/test/time/tableTree.spec.ts | Adds performance benchmark tests for table operations. |
| packages/dds/tree/src/test/time/.mocharc.cjs | Configures Mocha for execution time tests. |
| packages/dds/tree/src/test/testHelper.ts | Provides utility functions and type definitions for table trees. |
chentong7
force-pushed
the
tree-table-api
branch
from
4efc39e to
c885a49
Compare
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
There was a problem hiding this comment.
Tree already runs execution time tests using its root .mocharc.cjs file and some flags specified directly in the npm script. I like the idea of moving it to use a specific config file like this but then we should move to it for all execution time tests, not just the new ones, so we'll want to start by copying whatever the current root config file does, add the necessary settings for these tests, and update existing npm scripts that run them to point to the new file.
There was a problem hiding this comment.
Right. I haven't decided to update package.json or update the root .mocharc.js to includes local mocharc file. Will get back to this one later.
| } | ||
| } | ||
|
|
||
| const unsubscribeFromCommitAppliedEvent = treeView.events.on( |
There was a problem hiding this comment.
Subscribing before filling the tree will result in us building up a large revertible stack from the column/row/cell insertions that were part of initialization. I don't think that's what we want. The undo/redo tests have specific operations they want inserted into the stack(s).
We also probably don't want to incur the overhead of these subscriptions during the measurements. I think my recommendation would be:
- Don't track undo/redo stacks outside of undo/redo tests.
- Only start populating the undo/redo stacks immediately before we do the pre-test operations that we plan to undo/redo
- Unsubscribe from event after we've built up the stack(s) we need for the test / before we begin measurement.
| (commit, getRevertible) => { | ||
| if (getRevertible !== undefined) { | ||
| const revertible = getRevertible(onDispose); | ||
| if (commit.kind === CommitKind.Undo) { |
There was a problem hiding this comment.
This logic isn't quite right. One issue of note: if a non-undo commit is made, then we should clear the redo stack (otherwise we keep stale redo operations). See this code in our brainstorm example for a more standard way to set this up: https://github.com/microsoft/FluidExamples/blob/main/brainstorm/src/utils/undo.ts#L22
Description
This PR introduces performance benchmark tests for SharedTree's table-related APIs, focusing on standard operations. The benchmarks cover:
Insertion of rows, columns, and cells
Includes insertion at the middle of the table for rows and columns. Similarly, tests removal for rows and columns.
Test Result