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

Codemirror benchmarks #150

Merged
merged 28 commits into from
Dec 8, 2023
Merged
Show file tree
Hide file tree
Changes from 26 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
33 changes: 33 additions & 0 deletions .github/workflows/benchmark.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
name: Push benchmarks to grafana

on:
push:
branches: [main]

jobs:
benchmark:
name: Time benchmark
environment: grafana-api-key
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v3
with:
ref: main
path: main

- name: Setup antlr4
uses: ./main/.github/actions/setup-antlr4

- name: Install dependencies with frozen lock file
run: npm ci

- name: Build project
env:
NODE_OPTIONS: '--max_old_space_size=4096'
run: npm run build

- name: Benchmark
run: npm run benchmark --concurrency 1
env:
GRAFANA_API_KEY: ${{ secrets.GRAFANA_API_KEY }}
64 changes: 64 additions & 0 deletions .github/workflows/performance-regression.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
name: Performance Regression Test

on:
pull_request:
branches: [main]

jobs:
benchmark:
name: Time benchmark
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v3
with:
path: pr

- uses: actions/checkout@v3
with:
ref: main
path: main

- name: Setup antlr4
uses: ./main/.github/actions/setup-antlr4

- name: Install dependencies with frozen lock file for main branch
run: npm ci
working-directory: main

- name: Install dependencies with frozen lock file for pr branch
run: npm ci
working-directory: pr

- name: Build project for main
env:
NODE_OPTIONS: '--max_old_space_size=4096'
run: npm run build
working-directory: main

- name: Build project for pr
env:
NODE_OPTIONS: '--max_old_space_size=4096'
run: npm run build
working-directory: pr

- name: Run benchmark on main (baseline)
run: npm run benchmark | tee main-benchmarks.txt
working-directory: main/packages/language-support

- name: Run benchmark on pr
run: npm run benchmark | tee pr-benchmarks.txt
working-directory: pr/packages/language-support

- name: Compare benchmark result
uses: openpgpjs/github-action-pull-request-benchmark@v1
with:
name: 'Time benchmark'
tool: 'benchmarkjs'
pr-benchmark-file-path: pr/packages/language-support/pr-benchmarks.txt
base-benchmark-file-path: main/packages/language-support/main-benchmarks.txt
comment-on-alert: true
alert-threshold: '130%'
fail-on-alert: true
fail-threshold: '150%'
github-token: ${{ secrets.GITHUB_TOKEN }}
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ generated
*.jar
*.clinic
*.tsbuildinfo
*.npmrc
*.npmrc
benchmarks.txt
1 change: 1 addition & 0 deletions .nvmrc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
18.18.2
OskarDamkjaer marked this conversation as resolved.
Show resolved Hide resolved
53 changes: 40 additions & 13 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,12 @@
"dev-codemirror": "turbo run @neo4j-cypher/react-codemirror-playground#dev",
"lint": "eslint . --ext .ts",
"lint-fix": "eslint . --fix --ext .ts",
"format": "prettier --config .prettierrc.json '**/*.ts' --write"
"format": "prettier --config .prettierrc.json '**/*.ts' --write",
"benchmark": "turbo run benchmark --concurrency=1"
},
"engineStrict": true,
"engines": {
"node": ">=18.18.2"
},
"devDependencies": {
"@changesets/cli": "^2.26.2",
Expand Down
5 changes: 4 additions & 1 deletion packages/language-support/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,14 @@
"build-types": "tsc --emitDeclarationOnly --outDir dist/types",
"build-esm": "esbuild ./src/index.ts --bundle --format=esm --sourcemap --outfile=dist/esm/index.mjs",
"build-commonjs": "esbuild ./src/index.ts --bundle --format=cjs --sourcemap --outfile=dist/cjs/index.cjs",
"benchmark": "esbuild ./src/tests/benchmarks/benchmark.ts --bundle --outfile=dist/benchmark/benchmark.js && BENCHMARKING=true node dist/benchmark/benchmark.js",
"clean": "rm -rf {dist,src/generated-parser}",
"watch": "tsc -b -w",
"test": "jest"
},
"devDependencies": {
"@types/jest": "^29.5.5"
"@types/benchmark": "^2.1.5",
"@types/jest": "^29.5.5",
"benchmark": "^2.1.4"
}
}
3 changes: 2 additions & 1 deletion packages/language-support/src/parserWrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,8 @@ class ParserWrapper {
parse(query: string): EnrichedParsingResult {
if (
this.parsingResult !== undefined &&
this.parsingResult.query === query
this.parsingResult.query === query &&
process.env.BENCHMARKING !== 'true'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a fan of this cause if someone had this env variable defined (unlikely but still possible) then the behaviour of the language server changes.

I'd rather probably we had a method to clean the cache and we call that after every test of the benchmark?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wdyt about naming it something where it is more obvious the behavior of the language server changes, something like: BENCHMARK_CYPHER_LANGUAGE_SERVER?

I'm fine with having a method to clean the cache as well if you prefer

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wdyt about naming it something where it is more obvious the behavior of the language server changes, something like: BENCHMARK_CYPHER_LANGUAGE_SERVER?

I think explicitly cleaning the cache is a better idea IMO. The reason is right now I'm not sure whether this is benchmarking the right thing because in the auto-completion or the syntax validation we are also including another component, which is the parsing. So I'd rather we could control when to clean or not to clean the cache

Copy link
Collaborator

@ncordon ncordon Dec 7, 2023

Choose a reason for hiding this comment

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

I'm flexible, so if you prefer to do that on another PR, we can iterate on this. That way we get to test it out as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it'd be nice to do in a follow up PR, just to see that the Github actions work

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I went ahead and did it directly in this PR instead.

) {
return this.parsingResult;
} else {
Expand Down
Loading
Loading