Add GitHub Actions workflow for benchmarking and commenting on PR #269
Conversation
WalkthroughA new GitHub Actions workflow file has been added to automate benchmark execution on pull requests targeting the Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request
participant GH as GitHub Actions
participant Env as Environment Setup
participant Bench as Benchmark Job
participant Comm as Comment Manager
PR->>GH: Trigger workflow on PR targeting main
GH->>Env: Checkout repository & setup Node.js environment
Env->>GH: Environment ready
GH->>Bench: Install DFX, cache mops packages, install npm dependencies, build CLI, install mops tool
Bench->>Bench: Execute `mops bench` command
Bench->>Comm: Capture results and search existing bot comment
Comm->>PR: Create new comment or update existing comment with benchmark results
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
frontend/components/package/PackageRightPanel.svelteOops! Something went wrong! :( ESLint: 8.57.0 Error: Failed to load plugin 'svelte' declared in 'frontend/.eslintrc.json': Cannot find module 'svelte/compiler'
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/benchmarks.yml (2)
15-18: Matrix Configuration for Node.js VersionThe property
"node-version"is currently set to use\${{ matrix.node-version }}; however, there is no matrix strategy defined in the job. This means the variable may be undefined at runtime. Consider one of the following approaches:
- Define a matrix strategy: If you intend to test against multiple Node.js versions, add a
strategy.matrixblock specifying thenode-versionvalues.- Use a static value: If only one Node.js version is needed, replace the matrix variable with a literal (e.g.,
'16').Example diff if a static value is preferred:
- node-version: ${{ matrix.node-version }} + node-version: '16'🧰 Tools
🪛 actionlint (1.7.4)
17-17: property "node-version" is not defined in object type {}
(expression)
71-71: Ensure Newline at End-of-FileYAMLlint indicates that there is no newline character at the end of the file. Adding a newline at the end helps avoid formatting warnings and ensures compatibility with various tools.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 71-71: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/benchmarks.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/benchmarks.yml
17-17: property "node-version" is not defined in object type {}
(expression)
🪛 YAMLlint (1.35.1)
.github/workflows/benchmarks.yml
[error] 71-71: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: macos-latest, node 20, moc 0.11.1, ic-mops@1.0.0
- GitHub Check: ubuntu-latest, node 20, moc 0.11.1, ic-mops@1.0.0
Benchmark ResultsBenchmark files: Add (second)Add items one-by-one (second) Instructions
Heap
Garbage Collection
AddAdd items one-by-one Instructions
Heap
Garbage Collection
Arrayarr arr Instructions
Heap
Garbage Collection
Remove items using
|
| remove 70k | |
|---|---|
| Vector | 32_210_297 |
| Buffer | 31_716_422 |
Heap
| remove 70k | |
|---|---|
| Vector | -136.8 KiB |
| Buffer | -269.76 KiB |
Garbage Collection
| remove 70k | |
|---|---|
| Vector | 139.45 KiB |
| Buffer | 540.43 KiB |
Stable Memory and Region
Grow Region and store blobs in it
Instructions
| Region (fill 1/100) | Region (fill 1/50) | StableMemory | |
|---|---|---|---|
| 10 pages | 65_117_422 |
260_557_814 |
2_861 |
| 100 pages | 1_302_325_766 |
2_604_593_980 |
2_954 |
| 256 pages | 3_334_080_226 |
6_668_025_672 |
3_704 |
Heap
| Region (fill 1/100) | Region (fill 1/50) | StableMemory | |
|---|---|---|---|
| 10 pages | 296 B |
272 B |
276 B |
| 100 pages | 308 B |
308 B |
272 B |
| 256 pages | 308 B |
308 B |
276 B |
Garbage Collection
| Region (fill 1/100) | Region (fill 1/50) | StableMemory | |
|---|---|---|---|
| 10 pages | 2.38 MiB |
9.51 MiB |
336 B |
| 100 pages | 47.55 MiB |
95.1 MiB |
340 B |
| 256 pages | 121.73 MiB |
243.45 MiB |
340 B |
Stable Memory
| Region (fill 1/100) | Region (fill 1/50) | StableMemory | |
|---|---|---|---|
| 10 pages | 8 MiB |
8 MiB |
8 MiB |
| 100 pages | 8 MiB |
8 MiB |
0 B |
| 256 pages | 16 MiB |
16 MiB |
16 MiB |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/benchmarks.yml (1)
67-73: Comment Creation or Update and File Newline
The final step, which usespeter-evans/create-or-update-comment@v4, correctly updates or creates a comment with the benchmark results. Additionally, note that YAMLlint flagged a missing newline at the end of the file. Please ensure a newline is added at the end to conform to best practices.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 73-73: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
mops.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
.github/workflows/benchmarks.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/benchmarks.yml
17-17: property "node-version" is not defined in object type {}
(expression)
🪛 YAMLlint (1.35.1)
.github/workflows/benchmarks.yml
[error] 73-73: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run Benchmarks and Comment
- GitHub Check: node 20, moc 0.11.1, ic-mops@1.0.0
- GitHub Check: ubuntu-latest, node 20, moc 0.11.1, ./cli/dist
- GitHub Check: macos-latest, node 20, moc 0.11.1, ./cli/dist
🔇 Additional comments (10)
.github/workflows/benchmarks.yml (10)
1-2: Workflow Name Clarity
The workflow name "Comment Benchmarks" clearly reflects that its purpose is to execute benchmarks and post the results as comments on pull requests.
3-7: Trigger Configuration
The workflow is set to trigger on pull requests targeting the "main" branch, which aligns with the PR objectives. Verify that this trigger covers all intended scenarios, or if additional triggers (e.g., pushes to specific branches) might be needed in the future.
8-12: Job Definition and Environment Setup
The job "benchmark" is configured to run onubuntu-24.04and includes a clear, descriptive name. This is consistent with the intended environment for benchmark execution.
20-27: DFX Installation Steps
The steps to install the DFX tool, confirm its installation viadfx --version, and install the DFX cache are clear and appear to be in the correct order. Ensure that the version installed meets your project requirements.
29-37: Caching Mops Packages
The caching configuration for mops packages is well-structured using the hash of themops.tomlfile for the cache key. Please double-check that themops.tomlfile’s location is accurate relative to the repository root.
39-43: CLI Setup Steps
The steps to install npm packages in theclidirectory and build the local CLI usingnpm run prepareare straightforward and correctly sequenced.
45-46: Global Installation of Mops
Installing the mops CLI globally usingnpm i -g ./cli/distis a solid approach. Just ensure that the built CLI behaves as expected when installed globally.
48-49: Mops Packages Installation
The step to install mops packages withmops installis concise and clear, ensuring dependencies are in place before running benchmarks.
51-58: Benchmark Execution and Output Capture
The benchmark step executesmops benchand captures its output correctly by using a tee to save the results and then appending them to$GITHUB_OUTPUTin a here-doc style. Verify that the output format produced is compatible with downstream processing.
59-66: Existing Comment Lookup
Usingpeter-evans/find-comment@v3to locate an existing comment by the GitHub Actions bot is a good strategy to avoid duplicate comments. The selection criteria withcomment-authorandbody-includesare precise.
.github/workflows/benchmarks.yml
Outdated
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: ${{ matrix.node-version }} | ||
| cache: 'npm' | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Node.js Setup and Matrix Variable
The Node.js setup step uses node-version: ${{ matrix.node-version }} along with caching enabled for npm. However, no matrix is defined for this job, which could lead to runtime issues. Consider either defining a matrix strategy or replacing it with a static Node.js version. For example:
- node-version: ${{ matrix.node-version }}
+ node-version: '16'🏁 Script executed:
#!/bin/bash
# Search for any matrix definitions in the benchmarks workflow file
rg 'matrix:' .github/workflows/benchmarks.ymlLength of output: 45
Action Required: Define Matrix or Use a Static Node Version
The workflow currently uses the expression node-version: ${{ matrix.node-version }} without providing a corresponding matrix definition. This will likely cause runtime failures. You should either introduce a matrix strategy to supply the Node.js version or replace it with a static version. For instance:
- node-version: ${{ matrix.node-version }}
+ node-version: '16'- File:
.github/workflows/benchmarks.yml(Lines 14-19) - Issue: Undefined matrix leading to potential runtime issues
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v4 | |
| - uses: actions/setup-node@v4 | |
| with: | |
| node-version: ${{ matrix.node-version }} | |
| cache: 'npm' | |
| - uses: actions/checkout@v4 | |
| - uses: actions/setup-node@v4 | |
| with: | |
| node-version: '16' | |
| cache: 'npm' |
🧰 Tools
🪛 actionlint (1.7.4)
17-17: property "node-version" is not defined in object type {}
(expression)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/benchmarks.yml (2)
19-20: Consider pinning the DFX setup action to a specific version.Currently, the workflow uses
dfinity/setup-dfx@main, which refers to the latest version on the main branch. While this ensures you always get the latest features, it may introduce unexpected changes or breaking updates.- uses: dfinity/setup-dfx@main + uses: dfinity/setup-dfx@v0.3.0Replace
v0.3.0with the latest stable release version available. This will make your workflow more stable and reproducible.
72-72: Add a newline at the end of the file.The YAML linter detected that there is no newline character at the end of the file, which is a common convention for text files.
body: ${{ steps.benchmarks.outputs.result }} edit-mode: replace +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 72-72: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.bench/1-buffer-vector-add.bench.json(1 hunks).bench/2-vector-buffer-add.bench.json(1 hunks).bench/array.bench.json(1 hunks).bench/removeLast.bench.json(1 hunks).bench/stable-memory.bench.json(1 hunks).github/workflows/benchmarks.yml(1 hunks)cli/commands/bench.ts(3 hunks)
✅ Files skipped from review due to trivial changes (5)
- .bench/2-vector-buffer-add.bench.json
- .bench/removeLast.bench.json
- .bench/stable-memory.bench.json
- .bench/1-buffer-vector-add.bench.json
- .bench/array.bench.json
🧰 Additional context used
🧬 Code Definitions (1)
cli/commands/bench.ts (1)
cli/commands/test/reporters/compact-reporter.ts (1) (1)
res(75-99)
🪛 YAMLlint (1.35.1)
.github/workflows/benchmarks.yml
[error] 72-72: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ubuntu-latest, node 20, moc 0.11.1, ./cli/dist
- GitHub Check: macos-latest, node 20, moc 0.11.1, ./cli/dist
- GitHub Check: node 20, moc 0.11.1, ic-mops@1.0.0
🔇 Additional comments (9)
.github/workflows/benchmarks.yml (3)
17-17: Node.js setup now correctly uses caching without a matrix.The Node.js setup step correctly implements caching for npm dependencies without requiring a matrix definition. This resolves the issue from the previous review.
53-56: Good approach to capturing benchmark results.Using the
GITHUB_OUTPUTapproach with the heredoc syntax (<<EOF) is the recommended way to handle multi-line outputs in GitHub Actions, especially for benchmark results that may contain formatting.
58-72: Effective PR comment mechanism.The implementation for finding and updating comments is well-structured. Using
peter-evans/find-commentandpeter-evans/create-or-update-commenttogether ensures that benchmark results are properly posted as comments without creating duplicates.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 72-72: no new line character at the end of file
(new-line-at-end-of-file)
cli/commands/bench.ts (6)
266-266: Good initialization for tracking empty result sets.Adding the
allZerovariable to track whether all results for a property are zero is a good approach for determining whether to display the Stable Memory table.
274-276: Effective logic for tracking non-zero values.The code correctly updates the
allZeroflag when encountering any non-zero value, which will be used later to determine whether to display the Stable Memory section.
284-286: Good handling of NaN values in percentage calculations.Converting NaN results to 0 when calculating performance differences ensures that the output remains clean without unexpected "NaN%" displays.
290-296: Clever environment-specific formatting for benchmark results.The code smartly adapts the output format based on the environment:
- In CI environments: Uses LaTeX-style formatting for colors that will render properly in GitHub comments
- In local environments: Uses chalk for terminal-friendly colored output
This approach ensures good readability in both contexts.
323-326: Smart optimization to hide empty tables.Not displaying the Stable Memory table when all values are zero is a good UX decision that avoids cluttering the output with empty or meaningless tables.
343-343: Consistent conditional rendering.The conditional rendering of the Stable Memory section aligns with the
allZerotracking logic implemented earlier, creating a cleaner and more focused output.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cli/package.json (1)
60-60: Dependency Update: Commander bumped to 13.1.0This change updates the "commander" dependency from version "12.1.0" to "13.1.0". Please ensure that any CLI functionality—especially the parts interfacing with the newly added GitHub Actions benchmarking workflow—is compatible with the new version, as there may be breaking changes or altered APIs between these versions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cli/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
cli/package.json(1 hunks)frontend/components/package/PackageRightPanel.svelte(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ubuntu-latest, node 20, moc 0.11.1, ./cli/dist
- GitHub Check: macos-latest, node 20, moc 0.11.1, ./cli/dist
- GitHub Check: node 20, moc 0.11.1, ic-mops@1.0.0
🔇 Additional comments (1)
frontend/components/package/PackageRightPanel.svelte (1)
35-35:✅ Verification successful
Improved number readability with locale formatting.
Adding
toLocaleString()enhances the user experience by formatting large download numbers with appropriate separators, making them easier to read.To ensure this won't cause runtime errors, verify that
packageDetails.downloadsTotalis always a number:
🏁 Script executed:
#!/bin/bash # Check for type definitions or null checks for downloadsTotal rg "downloadsTotal.*:" -A 2 --type=ts rg "downloadsTotal.*=" -A 2Length of output: 2580
Verified Approval: Number Formatting and Type Safety Confirmed
The usage of
toLocaleString()in the<div class="value">forpackageDetails.downloadsTotalenhances number readability without introducing runtime errors. Our verification shows thatdownloadsTotalis defined as abigintin the type declarations (e.g., infrontend/declarations/main/main.did.d.ts), ensuring that it's always a numeric value. No additional null or error checks are needed.
- File:
frontend/components/package/PackageRightPanel.svelteat line 35- Change: Formatting
downloadsTotalwith.toLocaleString()- Verification: The type definition as
bigintconfirms safe usage without runtime errors.
Summary by CodeRabbit
New Features
Bug Fixes
Enhancements