-
Notifications
You must be signed in to change notification settings - Fork 35
AG-16239: Add high-frequency data benchmarks for bar, candlestick, OHLC, and line series #5550
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
Conversation
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.
💡 Codex Review
ag-charts/packages/ag-charts-community/benchmarks/highFrequencyDataLine.test.ts
Lines 87 to 114 in 9518efb
| benchmark( | |
| '10x append batch (1k points total)', | |
| ctx.repeatCount(10), | |
| { expectedRelativeMB: 0.5, expectedCanvasCount: 2, autoSnapshot: false }, | |
| async () => { | |
| const append = createBatch(BATCH_SIZE); | |
| data = data.concat(append); | |
| await (ctx.chart as any).applyTransaction({ append }); | |
| }, | |
| 30_000 | |
| ); | |
| benchmark( | |
| '1x remove batch (100 points)', | |
| ctx, | |
| { expectedRelativeMB: 0.5, expectedCanvasCount: 2, autoSnapshot: false }, | |
| async () => { | |
| const remove = data.slice(0, BATCH_SIZE); | |
| data = data.slice(BATCH_SIZE); | |
| await (ctx.chart as any).applyTransaction({ remove }); | |
| }, | |
| 15_000 | |
| ); | |
| benchmark( | |
| '1x rolling window update (append + remove)', | |
| ctx, | |
| { expectedRelativeMB: 0.5, expectedCanvasCount: 2, autoSnapshot: false }, |
After calling ctx.repeatCount(10) for the "10x append batch" test, the same BenchmarkContext is reused for the subsequent "1x remove" and "1x rolling window" cases without resetting repeat. repeatCount mutates the context and the benchmark helper reads ctx.repeat directly, so both "1x" tests are actually executed ten times each. That inflates timings and memory measurements and risks timeouts. Reset the repeat count (e.g. ctx.repeatCount(1)) before the single-run benchmarks, and apply the same fix to the bar/candlestick/ohlc high-frequency tests which follow the same pattern.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
After calling ctx.repeatCount(10) for the 10x append batch test, reset the repeat count to 1 before subsequent 1x tests to prevent them from running 10 times. This fixes inflated timings and memory measurements. Applied to all high-frequency benchmark files: line, bar, candlestick, and OHLC.
Fixed. |
manapeirov
left a comment
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.
LGTM
This PR adds comprehensive benchmarks for high-frequency data scenarios across multiple chart series types.
Changes
Reviewers
This PR should be reviewed by the Charts dev team as it focuses on benchmark additions and library functionality.