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

Support for synchronous benchmarks #202

Closed
rubennorte opened this issue Dec 10, 2024 · 10 comments · Fixed by #210
Closed

Support for synchronous benchmarks #202

rubennorte opened this issue Dec 10, 2024 · 10 comments · Fixed by #210

Comments

@rubennorte
Copy link
Contributor

rubennorte commented Dec 10, 2024

Hey folks! I'm an engineer working on React Native at Meta and we're doing some explorations about benchmarking React Native internals (this isn't meant for developers using React Native, but for the people maintaining React Native itself).

We started these explorations using tinybench but we have some limitations requiring parts of these tests to be fully synchronous (so no async functions or promises). Would you be open to a PR to support this use case? Basically a way to call bench.run so it runs them synchronously and returns void or the benchmark results directly, instead of a promise.

In that case, do you have any preferences on how to set that up? Some options I could think of are:

  1. Add a new runSync method that runs synchronously and ensures none of the configuration methods return promises.
  2. Make run return a promise or not based on whether any of the hooks provided promises. This feels like it could be complex to implement and it would easily switch from sync to async accidentally.

If you think this is something that would be valuable for your users overall, I'd happy to submit a PR implementing it. Otherwise we can set up an internal fork for this use case, so no pressure :).

Thanks!

@jerome-benoit
Copy link
Collaborator

jerome-benoit commented Dec 10, 2024

The API of the first proposal could be:

  • runAsync (private)
  • runSync (private)
  • run (public) that will call the appropriate run* method automatically or via a tunable

The async detection of a benchmark complete callbacks call chain could be automated (with known detection issues)

The runAsync method signature could even been the same as runSync one as long as no results are expected. The results will be then written async to the dedicated Task attribute.

@jerome-benoit
Copy link
Collaborator

After more thoughts:

  • runSync: public API to run the benchmark suite sync. Method signature: runSync: Task[]. A sanity check shall be executed on the suite callbacks chain at Task and Bench level to ensure it can be sync.
  • runAsync: public API to run the benchmark suite async without returning anything. Method signature: runAsync: void. The suite run results will be aggregated to the Tasks results. There's event already notifying for completion.
  • run: public API to run the benchmark suite accordingly to the 'syncness' status of the callbacks chain. Keep method signature for backward compatibility.

I guess the two last bullet points are enough to cover the need.

The implementation will share common code.

@rubennorte
Copy link
Contributor Author

I think having explicit runSync and runAsync (or just run for backwards compatibility) is simple enough, but how would you go about the automatic detection of "syncness"? AFAIK we can only do it reliably at runtime, checking if any of the hooks returned a promise, but doing that makes the code very complex, as you'd have to wrap/unwrap lots of code based on the status of the already executed hooks.

It seems making the API more explicit doesn't only improve reliability (if you need to run the tests synchronously, like our case, we want to avoid accidentally switching to async because someone made a change to a hook to make it async), but it also makes the implementation significantly simpler.

Any concerns about app size increase? Should we expose this as tinybench/sync to avoid increasing the size for everyone?

@jerome-benoit
Copy link
Collaborator

I think having explicit runSync and runAsync (or just run for backwards compatibility) is simple enough,

There's difference between the 3 methods that can all have their use cases.

but how would you go about the automatic detection of "syncness"? AFAIK we can only do it reliably at runtime, checking if any of the hooks returned a promise, but doing that makes the code very complex, as you'd have to wrap/unwrap lots of code based on the status of the already executed hooks.

The existing code is already partially doing it (with known detection issues). The infrastructure allowing to detect if a suite is using async callbacks or returning promises is already here and used in run() at Task and Bench level.

It seems making the API more explicit doesn't only improve reliability (if you need to run the tests synchronously, like our case, we want to avoid accidentally switching to async because someone made a change to a hook to make it async), but it also makes the implementation significantly simpler.

The API can enforce proper usage only if strongly typed. Meaning the enforcement will work only if TS is used. Improper usage of the sync API will still need sanity checks (even imperfect).

Any concerns about app size increase? Should we expose this as tinybench/sync to avoid increasing the size for everyone?

We can also have tinybench/sync and tinybench/async export namespace with the same API for each. I need to think about it but I'm not sure. The impact on the code refactoring need to be evaluated.

The code size should not increase a lot with common code sharing between implementations.

@rubennorte
Copy link
Contributor Author

The existing code is already partially doing it (with known detection issues). The infrastructure allowing to detect if a suite is using async callbacks or returning promises is already here and used in run() at Task and Bench level.

Oh, I just saw that. I think we'd have to run all the hooks before running the benchmark to detect if they're async and switch between the modes if that's the case. I'm a bit concerned about side-effects when doing that, because we'd be running all the hooks for all the benchmark cases before running any of them.

Would you be ok with just starting with runSync and explore the automatic detection in the future?

@jerome-benoit
Copy link
Collaborator

jerome-benoit commented Dec 11, 2024

Would you be ok with just starting with runSync and explore the automatic detection in the future?

Sure. The global API revamp discussion was just to have the big picture. Start with the part of it fulfilling your needs.

@Aslemammad
Copy link
Member

@rubennorte do you have discord? would love to create a discord group between @jerome-benoit, you and I!

@jerome-benoit
Copy link
Collaborator

jerome-benoit commented Dec 11, 2024

@Aslemammad: keeping the design discussion in a github issue is fine by me. It's public and anyone can participate.

@Aslemammad
Copy link
Member

oh yea totally, i wanted to discuss more private stuff! i'm an advocate for indexable github issues :)

@rubennorte
Copy link
Contributor Author

rubennorte commented Dec 11, 2024

@rubennorte do you have discord? would love to create a discord group between @jerome-benoit, you and I!

yeah, it's ruben.norte there

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 a pull request may close this issue.

3 participants