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

feat: compiler benchmarks and gh action #3503

Merged
merged 36 commits into from
May 4, 2022
Merged

feat: compiler benchmarks and gh action #3503

merged 36 commits into from
May 4, 2022

Conversation

MrArnoldPalmer
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer commented Apr 28, 2022

Adds a private package @jsii/benchmarks which includes a basic command line application for running jsii benchmarks.

Adds a custom benchmark runner leveraging the NodeJS perf_hooks module to time function calls across multiple iterations and return averaged results.

Adds github action workflows to run benchmarks on PRs and compare performance to the target branch. This action will fail when a test suite is slower by 200%, but this threshold can be configured.

Adds to the gh-pages action workflow to run benchmarks and append new results to a file that is displayed as a graph on our docs site at the /dev/bench url. This will show benchmark suite results over time and allow us to track overall change in compiler performance across multiple commits.

Example of benchmark results comment MrArnoldPalmer#417 (comment)
Example of benchmark results graph (but only 1 result cause just testing) https://mrarnoldpalmer.github.io/jsii/dev/bench/


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@MrArnoldPalmer MrArnoldPalmer requested review from RomainMuller and a team April 28, 2022 03:15
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Apr 28, 2022
.github/workflows/gh-pages.yml Outdated Show resolved Hide resolved
.github/workflows/gh-pages.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/gh-pages.yml Outdated Show resolved Hide resolved
packages/@jsii/benchmarks/package.json Outdated Show resolved Hide resolved
packages/@jsii/benchmarks/index.ts Outdated Show resolved Hide resolved
packages/@jsii/benchmarks/index.ts Outdated Show resolved Hide resolved
packages/@jsii/benchmarks/index.ts Outdated Show resolved Hide resolved
packages/@jsii/benchmarks/index.ts Outdated Show resolved Hide resolved
packages/@jsii/benchmarks/index.ts Outdated Show resolved Hide resolved
@MrArnoldPalmer MrArnoldPalmer added the pr/do-not-merge This PR should not be merged at this time. label Apr 29, 2022
@MrArnoldPalmer
Copy link
Contributor Author

Added DNM as benchmark.js doesn't really support what we want to do. Writing a basic custom runner that generates output that the action we use supports so we should still get the graphs on gh-pages and the comments for regression exceeding defined threshold.

@MrArnoldPalmer MrArnoldPalmer removed the pr/do-not-merge This PR should not be merged at this time. label May 2, 2022
@MrArnoldPalmer
Copy link
Contributor Author

Changed strategy a bit since last review @RomainMuller @yuth

  1. Removed benchmark.js for a really simple custom runner using the node perf_hooks module. Benchmark.js wasn't cutting it for us as it isn't really built around running tests that may require cleanup in between. There are no apis for cleaning up the target directory in between runs and therefore after the first run, JSII essentially no-ops, skewing the results.

  2. Added a fixtures directory that has a snapshot of the aws-cdk-lib project ready to build (meaning you can run npm install and then JSII is able to compile it). This greatly reduces the time of running the benchmark as cloning the repo, building all the submodules, running ubergen, removing build artifacts, THEN running JSII, is a lot of setup. I did include the script I used to snapshot it so we can rerun that or add new versions of cdk, or other modules, though some of it is aws-cdk-lib specific).

I also investigated including profiles and graphing difference in cpu/memory usage as well but it's non-trivial and we can add it after the fact if that data is interesting to us.

Copy link
Contributor

@yuth yuth left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@RomainMuller RomainMuller left a comment

Choose a reason for hiding this comment

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

A bunch of cosmetic feedback, feel free to ship it after having reviewed that...

.gitattributes Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
packages/@jsii/benchmarks/README.md Outdated Show resolved Hide resolved
packages/@jsii/benchmarks/bin/benchmark.ts Outdated Show resolved Hide resolved
path.join(os.tmpdir(), 'jsii-cdk-bench-snapshot'),
);
cp.execSync(`tar xf ${cdkv2_21_1}`, { cwd: sourceDir });
cp.execSync('npm ci', { cwd: sourceDir });
Copy link
Contributor

Choose a reason for hiding this comment

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

npm ci? Isn't CDK using yarn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is, but the snapshot we are taking is outside of the yarn workspace and we run npm install. we can alternatively do yarn install and generate a yarn.lock but this assumes less about the environment?

packages/@jsii/benchmarks/lib/index.ts Outdated Show resolved Hide resolved
packages/@jsii/benchmarks/lib/index.ts Outdated Show resolved Hide resolved
packages/@jsii/benchmarks/package.json Outdated Show resolved Hide resolved
packages/@jsii/benchmarks/package.json Outdated Show resolved Hide resolved
@RomainMuller RomainMuller added the pr/do-not-merge This PR should not be merged at this time. label May 4, 2022
@MrArnoldPalmer MrArnoldPalmer removed the pr/do-not-merge This PR should not be merged at this time. label May 4, 2022
@MrArnoldPalmer MrArnoldPalmer added the pr/do-not-merge This PR should not be merged at this time. label May 4, 2022
Support async setup functions and use `tar` with a stream instead of
shelling out to avoid potential cross-platform issues and OOM errors.
@MrArnoldPalmer MrArnoldPalmer removed the pr/do-not-merge This PR should not be merged at this time. label May 4, 2022
@mergify
Copy link
Contributor

mergify bot commented May 4, 2022

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label May 4, 2022
@mergify
Copy link
Contributor

mergify bot commented May 4, 2022

Merging (with squash)...

@mergify mergify bot merged commit 4a91cf0 into main May 4, 2022
@mergify mergify bot deleted the feat/compiler-bench branch May 4, 2022 22:51
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants