Track time spent in each wasm instance env call #427
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kulakowski
commented
Oct 13, 2023
| @@ -0,0 +1,69 @@ | |||
| use std::time::{Duration, Instant}; | |||
There was a problem hiding this comment.
The span stuff is rough and ready, happy to do whatever with it once we agree this is all the right direction.
kulakowski
commented
Oct 13, 2023
| } | ||
|
|
||
| /// Call the function `f` with the name `func`. | ||
| /// The function `f` is provided with the callers environment and the host's memory. | ||
| /// | ||
| /// One of `cvt`, `cvt_ret`, or `cvt_noret` should be used in the implementation of any |
There was a problem hiding this comment.
What do you think of this structure?
There was a problem hiding this comment.
This seems like an improvement; I'm a fan.
kulakowski
force-pushed
the
george/wasm-instance-env-time
branch
2 times, most recently
from
e0e5a29 to
064564d
Compare
gefjon
approved these changes
Oct 16, 2023
Comment on lines
+631
to
+630
| log::debug!( | ||
| "Long running reducer {func_ident:?} took {:?} to execute, with the following wasm_instance_env call times: {:?}", | ||
| timings.total_duration, | ||
| timings.wasm_instance_env_call_times | ||
| ); |
There was a problem hiding this comment.
Ideally I think we should change this message so that it only tries to print the wasm_instance_env_call_times if we're collecting that data. I'm not concerned about that; maybe just a TODO comment?
| } | ||
|
|
||
| /// Call the function `f` with the name `func`. | ||
| /// The function `f` is provided with the callers environment and the host's memory. | ||
| /// | ||
| /// One of `cvt`, `cvt_ret`, or `cvt_noret` should be used in the implementation of any |
There was a problem hiding this comment.
This seems like an improvement; I'm a fan.
Additionally, this further tidies the interface between the host and the wasm instance env. In particular, reducer calls are explicitly started and stopped, and the wasm instance env becomes responsible for its own instrumentation.
Co-authored-by: Phoebe Goldman <phoebe@clockworklabs.io> Signed-off-by: George Kulakowski <george.kulakowski@gmail.com>
kulakowski
force-pushed
the
george/wasm-instance-env-time
branch
from
6adcb3a to
afff6e8
Compare
Co-authored-by: Phoebe Goldman <phoebe@clockworklabs.io> Signed-off-by: George Kulakowski <george.kulakowski@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Additionally, this further tidies the interface between the host and the wasm instance env. In particular, reducer calls are explicitly started and stopped, and the wasm instance env becomes responsible for its own instrumentation.
Description of Changes
API and ABI
If the API is breaking, please state below what will break
Expected complexity level and risk
2: This moderately changes some of the plumbing between the host and instance.
How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.
This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.
If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.