Skip to content
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

Attempt to get perf testing work #1532

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions .github/workflows/perf.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
name: PerformanceCheck

on:
workflow_dispatch:
push:
branches:
- main
pull_request:
branches: [main]

env:
EXPERIMENT_BRANCH_NAME: ${{ github.head_ref || github.ref_name }}
CONTROL_BRANCH_NAME: 'main'
FIDELITY: 100
FORK_NAME: ${{ github.event.pull_request.head.repo.full_name }}

jobs:
master-krausest-comparison:
name: Glimmer Krausest Benchmark
runs-on: ubuntu-latest
timeout-minutes: 20
steps:
- uses: actions/checkout@v4
with:
fetch-depth: 0
- uses: wyvox/action-setup-pnpm@v3
with:
node-version: '20.1.0'

- name: RUN
run: pnpm run benchmark:setup

- name: Upload Tracerbench Artifacts
if: failure() || success()
uses: actions/upload-artifact@v3
with:
name: Trace Artifacts
path: tracerbench-results

- name: Write message
uses: mshick/add-pr-comment@v2
with:
message-path: "tracerbench-results/msg.txt"
update-only: true
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@ tmp/
instrumentation.*.json
.cache
**/*.tgz
tracerbench-results
27 changes: 26 additions & 1 deletion benchmark/benchmarks/krausest/lib/index.js
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excellent, so in a future PR we could make different render calls or click around and measure the results afterwards.

like, some things we probably want to measure:

  • appending 1 row
  • appending 1000 rows
  • clearing all rows
  • switching two rows' positions
  • updating data within one row
    • ideally this doesn't cause every row to re-compute, even though I think today it does... 🙈 (depends on where the data is located)
  • probably other stuff as well

Copy link
Contributor Author

@lifeart lifeart Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, all right. We may need to explore/cleanup existing logic (from prev bench, located https://github.com/glimmerjs/glimmer-vm/tree/main/packages/%40glimmer-workspace/benchmark-env)

Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,31 @@ export default async function render(element, isInteractive) {
const args = {
items: buildData(),
};

enforcePaintEvent();
performance.mark('glimmer-render-1000-rows-start');
enforcePaintEvent();
await benchmark.render('Application', args, element, isInteractive);
performance.mark('glimmer-render-1000-rows-finished');
enforcePaintEvent();
}

function enforcePaintEvent() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we know why this exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NullVoxPopuli, yep, tracerbench require to have paint event after last mark (TracerBench/tracerbench#305), we may remove it in future, once proper events will be settlled-up and no issues spotted.

const docElem = document.documentElement;
const refNode = docElem.firstElementChild || docElem.firstChild;
const fakeBody = document.createElement('body');
const div = document.createElement('div');

div.id = 'mq-test-1';
div.style.cssText = 'position:absolute;top:-100em';
fakeBody.style.background = 'none';
fakeBody.appendChild(div);
div.innerHTML = '&shy;<style> #mq-test-1 { width: 42px; }</style>';
docElem.insertBefore(fakeBody, refNode);

try {
return div.offsetWidth === 42;
} finally {
fakeBody.removeChild(div);
docElem.removeChild(fakeBody);
}
}
2 changes: 2 additions & 0 deletions benchmark/benchmarks/krausest/package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
{
"private": true,
"name": "@glimmer-workspace/krausest",
"type": "module",
"scripts": {
"start": "vite --mode production",
"test:lint": "eslint .",
"test:types": "tsc --noEmit -p ../../tsconfig.json"
},
Expand Down
29 changes: 29 additions & 0 deletions benchmark/benchmarks/krausest/vite.config.mts
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,44 @@ import fs from 'node:fs';

import { precompile } from '@glimmer/compiler';
import { defineConfig, type Plugin } from 'vite';
import path from 'node:path';
import { fileURLToPath } from 'node:url';

const self = import.meta.url;

const packagesPath = path.resolve(path.dirname(fileURLToPath(self)), '..', '..', './../packages');

const packagePath = (name: string) => {
return path.join(packagesPath, name, 'dist/prod/index.js');
};

export default defineConfig({
plugins: [benchmark()],
resolve: {
alias: {
'@glimmer-workspace/benchmark-env': '@glimmer-workspace/benchmark-env/index.ts',
'@glimmer/debug': packagePath('@glimmer/debug'),
},
},
});

function benchmark(): Plugin {
return {
enforce: 'pre',
name: '@glimmer/benchmark',
resolveId(id) {
if (id === '@glimmer/env') {
return '\0@glimmer/env';
} else if (id === '@glimmer/local-debug-flags') {
return '\0@glimmer/local-debug-flags';
}
},
load(id) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this needed? the build / package.json#exports should have already handled this 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see it was pre-existing from here: https://github.com/glimmerjs/glimmer-vm/pull/1532/files#diff-860eda2adf31f087997e1c92eab487cb90113b3f303653d36adabd42213d5c42L26

We can probably evaluate removing / cleaning it up in a separate PR then.

if (id === '\0@glimmer/env') {
return `export const DEBUG = false;`;
} else if (id === '\0@glimmer/local-debug-flags') {
return `export const LOCAL_SHOULD_LOG = false;`;
}
/** @type {string | undefined} */
let result: string | undefined;
if (id.endsWith('.hbs')) {
Expand Down
3 changes: 0 additions & 3 deletions benchmark/bin/build.js

This file was deleted.

12 changes: 0 additions & 12 deletions benchmark/bin/control.js

This file was deleted.

12 changes: 0 additions & 12 deletions benchmark/bin/experiment.js

This file was deleted.

127 changes: 0 additions & 127 deletions benchmark/lib/build.js

This file was deleted.

47 changes: 0 additions & 47 deletions benchmark/lib/rollup-plugin.js

This file was deleted.

17 changes: 2 additions & 15 deletions benchmark/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,9 @@
"version": "0.84.3",
"private": true,
"dependencies": {
"@glimmer-workspace/benchmark-env": "workspace:^",
"@simple-dom/document": "^1.4.0",
"@simple-dom/serializer": "^1.4.0",
"@simple-dom/void-map": "^1.4.0"
"@glimmer-workspace/benchmark-env": "workspace:^"
},
"devDependencies": {
"@glimmer/compiler": "workspace:^",
"@rollup/plugin-strip": "^3.0.4",
"@rollup/plugin-terser": "^0.4.4",
"@types/express": "^4.17.21",
"@types/fs-extra": "^11.0.4",
"@types/symlink-or-copy": "^1.2.2",
"express": "^4.18.2",
"fs-extra": "^11.1.1",
"rollup": "^4.5.1",
"rollup-plugin-sourcemaps": "^0.6.3",
"symlink-or-copy": "^1.3.1"
"@glimmer/compiler": "workspace:^"
}
}
1 change: 1 addition & 0 deletions benchmark/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"module": "esnext",
"moduleResolution": "bundler",
"verbatimModuleSyntax": true,
"noErrorTruncation": true,

"suppressImplicitAnyIndexErrors": false,
"useDefineForClassFields": false,
Expand Down
Loading
Loading