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

benchmarks #121

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

benchmarks #121

wants to merge 16 commits into from

Conversation

krisbitney
Copy link
Contributor

This PR adds a benchmark package to measure the performance of various client methods

@codecov
Copy link

codecov bot commented Jun 13, 2023

Codecov Report

Patch coverage: 2.22% and project coverage change: -1.28 ⚠️

Comparison is base (c68de6e) 59.89% compared to head (41f94f9) 58.61%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #121      +/-   ##
==========================================
- Coverage   59.89%   58.61%   -1.28%     
==========================================
  Files          82       84       +2     
  Lines        4266     4311      +45     
==========================================
- Hits         2555     2527      -28     
- Misses       1711     1784      +73     
Impacted Files Coverage Δ
packages/benchmarks/src/fibonacci.rs 0.00% <0.00%> (ø)
packages/benchmarks/src/lib.rs 5.88% <5.88%> (ø)

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@krisbitney krisbitney requested review from cbrzn, nerfZael, namesty, pileks, dOrgJelli and Niraj-Kamdar and removed request for cbrzn and nerfZael June 13, 2023 12:52
@cbrzn cbrzn marked this pull request as draft June 19, 2023 12:42
let http_uri = UriCase {
id: "http_uri".to_string(),
uri: Uri::try_from(format!(
"http/https://raw.githubusercontent.com/polywrap/rust-client/kris/debug-slow-invocation/packages/benchmarks/fibonacci/rs/build"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should create a benchmark-wraps repo in the polywrap organization and use that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, sounds good! I'll do that soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to clarify, are you suggesting we no longer benchmark fs resolution, or that we should write a script to clone the benchmark-wraps repo temporarily in order to test fs resolution?


pub fn get_fibonacci_wrap(implementation: &str) -> WasmWrapper {
let path = Path::new(&get_fibonacci_dir(implementation)).join("wrap.wasm");
let module_bytes = fs::read(path).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use include_bytes! for this instead of the file system to get some performence increase. Check out how it's done in the default-config crate.
Note: you wont be able to pass in the implementation str, but that's an ok compromise IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's awesome. This isn't affecting the benchmarks. Do you think the change still needs to be made?

Copy link
Contributor

@nerfZael nerfZael Jun 21, 2023

Choose a reason for hiding this comment

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

I think so, if it's not too much trouble. It makes sense cause it decreases time it takes for the tests to run (so developers can confirm their code works faster, CI is faster, etc). Ideally, we'd use include_bytes everywhere instead of fs::read everywhere. Ofc, one-off uses don't affect much :D

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