-
Notifications
You must be signed in to change notification settings - Fork 3
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
base: main
Are you sure you want to change the base?
benchmarks #121
Conversation
…ation # Conflicts: # Cargo.toml
Codecov ReportPatch coverage:
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
☔ View full report in Codecov by Sentry. |
…ation # Conflicts: # packages/default-config/Cargo.toml
…ation # Conflicts: # Cargo.toml # packages/builder/src/helpers.rs # packages/client/tests/test_map_type.rs
…ation # Conflicts: # Cargo.toml
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" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
This PR adds a benchmark package to measure the performance of various client methods