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: add support for embedded runner #6

Merged
merged 19 commits into from
Dec 12, 2024
Merged

Conversation

nazarhussain
Copy link

@nazarhussain nazarhussain commented Dec 4, 2024

Motivation

Make the benchmark to support multiple runtime.

Description

  • Remove mocha as peer dependency
  • Use a runner embedded into the package

Steps to test or reproduce

  • Run all tests

Copy link

github-actions bot commented Dec 4, 2024

Performance Report

✔️ no performance regression detected

Full benchmark results
Benchmark suite Current: d88b852 Previous: - Ratio
sum array with raw for loop 928.48 us/op
sum array with reduce 10.214 ms/op
sum array with reduce beforeEach 10.235 ms/op
sum array with reduce before beforeEach 10.252 ms/op
sum array with reduce high threshold 10.148 ms/op
sum array with reduce no threshold 10.188 ms/op

by benchmarkbot/action

@wemeetagain
Copy link
Member

pls update the readme

@nazarhussain
Copy link
Author

pls update the readme

There is not a lot to be reflected in the Readme. The breaking changes I have in my notes and will add to the release notes when we release these changes.

package.json Show resolved Hide resolved
src/index.ts Outdated

export {bench, itBench, setBenchOpts, setBenchmarkOptions} from "./benchmark/index.js";
export const describe = suite;
export const it = test;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to export it. Should we only expose the bench and itBench??

Copy link
Author

Choose a reason for hiding this comment

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

In few examples and usages, we have test suits which have mix of bench and normal tests. So we need to export it.

Copy link
Member

Choose a reason for hiding this comment

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

Silly question, but if we are using @vitest/runner is that the same functionality for it as would be expected from a standard test suite?

Copy link
Author

Choose a reason for hiding this comment

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

Yes it is but then you have to add vitest in your dependencies along with the @chainsafe/benchmark package.

Copy link
Member

@matthewkeil matthewkeil Dec 9, 2024

Choose a reason for hiding this comment

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

I think the consumer should import it from the testing library then. Seems a better pattern. If they need the peer dependency anyway then let them just use that

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why this package should export it, I don't see use using it inside Lodestar either, what's the use case?

And are we worried about backward compatiblity? as I understand we want to publish this as @chainsafe/benchmark which would be a new package

Copy link
Member

@wemeetagain wemeetagain Dec 9, 2024

Choose a reason for hiding this comment

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

This library is really tied to vitest/runner. I think how/if we go about re-exporting from vitest depends on what the library is trying to achieve.
Is this an extension to vitest to support benchmarks (where consumers can use vitest features + our benchmark features)?
Or is this an opinionated benchmarking tool with an API that merely mirrors a subset of vitest?

If its a vitest extension, maybe we shouldn't re-export anything from vitest runner (vitest/runner treated as a peer dependency) (or alternatively, we should re-export everything (vitest/runner treated as a dependency)).
If its an opinionated benchmarking tool, we should just re-export what we specifically need.

Also, we want describe, do we want to support afterEach/afterAll, etc?

Copy link
Author

@nazarhussain nazarhussain Dec 9, 2024

Choose a reason for hiding this comment

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

Is this an extension to vitest to support benchmarks (where consumers can use vitest features + our benchmark features)?

Vitest have it's own (benchmarking feature)[https://vitest.dev/guide/features.html#benchmarking] and our tool does not have anything to do do with it.

Or is this an opinionated benchmarking tool with an API that merely mirrors a subset of vitest?

Yes it's an independant tool which have a DSL for describe and it similar to vitest as well as mocha.

If its a vitest extension, maybe we shouldn't re-export anything from vitest runner

No it's not a vitest extension, if it was then it would not have it's own binary to run.

Also, we want describe, do we want to support afterEach/afterAll, etc?

We have similar callbacks supported in itBench options as before and beforeEach.

Copy link
Member

Choose a reason for hiding this comment

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

Great question on the before, beforeEach, after and afterEach as standalone functions as that is the common way to use those

Copy link
Author

Choose a reason for hiding this comment

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

For benchmarks you specify before or beforeEach as the options in itBench. It's the syntax from start for this library.

https://github.com/ChainSafe/ssz/blob/b85c774a2ed2a093ab36209aa3988feee56d8fcc/packages/persistent-merkle-tree/test/perf/hasher.test.ts#L38

https://github.com/ChainSafe/ssz/blob/b85c774a2ed2a093ab36209aa3988feee56d8fcc/packages/persistent-merkle-tree/test/perf/hasher.test.ts#L42

And probable reason is that for bench marks you have specific actions for each benchmark test. And afterEach is not required bas everything is discarded anyway after the benchmark run.

src/cli/options.ts Show resolved Hide resolved
src/cli/options.ts Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
src/benchmark/benchmarkFn.ts Show resolved Hide resolved
src/benchmark/benchmarkFn.ts Outdated Show resolved Hide resolved
src/benchmark/benchmarkFn.ts Outdated Show resolved Hide resolved
src/utils/output.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
Copy link
Member

@matthewkeil matthewkeil left a comment

Choose a reason for hiding this comment

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

Left some comments and questions.

@matthewkeil
Copy link
Member

Can we also get rid of the warnings when we start the runner?

❯ yarn benchmark
yarn run v1.22.22
$ node --loader ts-node/esm ./src/cli.ts 'test/perf/**/*.test.ts'
(node:11389) ExperimentalWarning: `--experimental-loader` may be removed in the future; instead use `register()`:
--import 'data:text/javascript,import { register } from "node:module"; import { pathToFileURL } from "node:url"; register("ts-node/esm", pathToFileURL("./"));'
(Use `node --trace-warnings ...` to show where the warning was created)
(node:11389) [DEP0180] DeprecationWarning: fs.Stats constructor is deprecated.
(Use `node --trace-deprecation ...` to show where the warning was created)
Connected to historyProvider: LocalHistoryProvider, dirpath ./benchmark_data
(node:11389) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
No previous bencharmk found for latestCommitInBranch 'main'

@nazarhussain
Copy link
Author

nazarhussain commented Dec 9, 2024

These are from the NodeJS, can be disabled externally with NODE_OPTIONS="--now-warnings" and adding.

No, i mean can we update this to remove that warning

"benchmark": "node --loader ts-node/esm ./src/cli.ts 'test/perf/**/*.test.ts'",

Copy link
Member

@matthewkeil matthewkeil left a comment

Choose a reason for hiding this comment

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

LGTM!! 🎸

@nazarhussain nazarhussain merged commit 56ae4c1 into main Dec 12, 2024
9 checks passed
@nazarhussain nazarhussain deleted the nh/embeded-runner branch December 12, 2024 16:03
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.

4 participants