-
Notifications
You must be signed in to change notification settings - Fork 336
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
Revive profiler, add docs, cleanups #518
Conversation
Benchmark for Linux-cuda a723e45Click to hide benchmark
Benchmark for Linux-default a723e45Click to hide benchmark
Benchmark for macOS-default a723e45Click to hide benchmark
Benchmark for macOS-metal a723e45Click to hide benchmark
|
@@ -40,6 +40,13 @@ struct Args { | |||
/// Add environment vairables in the form of NAME=value. | |||
#[clap(long, action = clap::ArgAction::Append)] | |||
env: Vec<String>, | |||
|
|||
/// Write "pprof" protobuf output of the guest's run to this file. |
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.
It might be nice to show a end-to-end example set of commands in a r0vm
crate level README ( and/or landing page for crates.io) of how to use profiling / other features of r0vm so its a little more accessible to users.
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.
Agreed, see: #472
pub use self::receipt::{SegmentReceipt, SessionReceipt}; | ||
#[cfg(feature = "prove")] | ||
pub use self::{ | ||
exec::io::{Syscall, SyscallContext}, |
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.
yay! Would it make sense to write a MultiTest for using a custom syscall? I would be interested in writing it if we need something like that to test the user facing API.
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.
MultiTest doesn't really test the external API, it really just tests the host/guest interaction.
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'm also not super keen on exposing the Syscall
interface directly, I think there are usually better ways to extend the host/guest (like using I/O). If we can drop this re-export I'd prefer to.
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 am also happy with 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.
Looks good!
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.
Great!
No description provided.