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

Benchmarking #261

Closed

Conversation

jonathantneal
Copy link
Contributor

@jonathantneal jonathantneal commented Oct 6, 2020

This request adds benchmarking abilities to the project using Continuous Benchmark with BenchmarkJS.

Highlights

  • Continuous integration. Benchmarks run on all pushes or PRs to the canary branch.
  • TypeScript. Benchmarks can run from ts files.
  • Developer Experience. Benchmarks run in *.benchmark.ts files similar to how tests run in *.test.ts files.
  • Visualized Performance. Tests are compared across commits and visually charted. Warnings are automatically added as comments to PRs if they introduce significant regressions.

Performance regressions that exceed 25% above the current threshold will result in an automatic warning added as a comment to a PR. Performance regressions that double the current threshold trigger a workflow failure.

Usage

Example (packages/core/benchmarks/index.benchmark.ts):

import { tokenizeValue } from '../src/shorthand-parser/value-tokenizer';

describe('Tokenizing', () => {
  test('Undefined', () => {
    tokenizeValue(undefined as any);
  });

  test('Two Ident Tokens', () => {
    tokenizeValue('solid red');
  });
});

Details

Benchmarking Purposes

These benchmarks can serve at least two purposes.

First, benchmarks can compare the performance of stitches with similar tooling. For them to be the most meaningful, they should demonstrate the strong and weak areas in similar tooling. While stitches may already be the “fastest” ⚡, developers would likely appreciate it if the benchmarks demonstrate where the other tooling does well and where it can improve performance.

Second, benchmarks can compare the performance of internal functions in order to measure improvements and regressions in individual pull requests and even between releases. These are especially helpful for developers to measure the most computationally intensive areas of the library.

Requirements

  1. We would need to write benchmarking scripts. Similar to the tests directory, benchmark scripts could be written to a benchmarks directory. Initially, these scripts would benchmark internal logic; again, mimicking the tests.
  2. We would need to host the visualized benchmark.
  3. We would need to decide how much or how little noise this action should add to a PR. Currently, codesandbox adds a comment below this with information about the current build. I personally find this to be noisy, and so adding another feels even more noisy. I prefer when these statuses are pushed into checks or as generated shield icons in the PR template.

Questions

  1. Which parts of the library should we be testing for performance in pushes and PRs? Which parts of the library should be testing for performance alongside other libraries?
  2. How many benchmarks or how much benchmark coverage should we seek to add in our first pass?
  3. How do we feel about benchmarks reusing describe() and test()?
  4. How do we feel about benchmarks being addable with any *.benchmark.ts?
  5. How do we feel about hosting the logic for these things in the repository versus an abstracted library?

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 6, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6fe698b:

Sandbox Source
Stitches CI: Next.js Configuration
Stitches CI: CRA Configuration

@jonathantneal jonathantneal changed the title Add benchmarks Benchmarking Oct 6, 2020
@jonathantneal jonathantneal marked this pull request as draft October 6, 2020 12:34
@peduarte
Copy link
Contributor

peduarte commented Oct 6, 2020

Excellent start 💯

For them to be the most meaningful, they should demonstrate the strong and weak areas in similar tooling

Yeah, definitely 👍 Are you planning on adding similar tooling in this PR too? (Styled components?)

Second, benchmarks can compare the performance of internal functions in order to measure improvements and regressions in individual pull requests and even between releases.

Love this <3 This will help us make decisions backed by meaningful data

Which parts of the library should we be testing for performance in pushes and PRs?

I'd think we need to benchmark the functions that are likely to be called the most often

  • css
  • styled
  • createCssRule
  • compose
  • createAtom
  • processStyleObject

Which parts of the library should be testing for performance alongside other libraries?

When we compare with other libraries, does it make sense to only compare "similar" API's? For example, the styled function from Stitches vs the styled function from styled-components?

How do we feel about benchmarks reusing describe() and test()?

No strong preference. Would they help keep the benchmarks more readable? If so, I'm all for it ✨

How do we feel about benchmarks being addable with any *.benchmark.ts?

Works for me 👌

How do we feel about hosting the logic for these things in the repository versus an abstracted library?

I'm happy for it to be in the repo for now, unless there's a strong reason for it to be abstracted

@jonathantneal jonathantneal force-pushed the jn.add-benchmarking branch 4 times, most recently from 85fa67d to 3dba0ff Compare October 10, 2020 04:57
@peduarte peduarte mentioned this pull request Oct 12, 2020
@jonathantneal jonathantneal force-pushed the jn.add-benchmarking branch 4 times, most recently from f1dcc7c to 9d397eb Compare October 12, 2020 15:52
@jonathantneal jonathantneal changed the base branch from canary to jn.add-benchmarking October 12, 2020 16:20
@jonathantneal jonathantneal changed the base branch from jn.add-benchmarking to canary October 12, 2020 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants