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

Add trace functionality #1642

Closed
wants to merge 1 commit into from
Closed

Add trace functionality #1642

wants to merge 1 commit into from

Conversation

maurolacy
Copy link
Contributor

@maurolacy maurolacy commented Mar 20, 2023

Related to #746.

Useful for comparing wall time between contract code and blockchain code. This way the latency of the Go -> Rust and Rust -> Go bridges can be easily measured (and eventually improved).

Useful as well for general benchmarks of code and procedures. Elapsed time can be computed post-facto by subtracting calls to trace().

The performance of code between the native and wasm runtimes cannot be compared fairly, as the WebAssembly code is running inside the VM. But time measurements are useful as a relative reference for comparing different routines and implementations in contract code.

It could also be interesting to know how much faster native code is for particular tasks, compared to wasm code.

Copy link
Contributor

@ueco-jb ueco-jb left a comment

Choose a reason for hiding this comment

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

What's the difference between debug and trace here?

@maurolacy
Copy link
Contributor Author

maurolacy commented Mar 21, 2023

What's the difference between debug and trace here?

The timestamp information. We can conflate the two things into one, but I think having two different calls makes sense / is more flexible.
debug can be used for printing the content of variables and stuff, whereas trace can be used for printing labels and timestamps. It's cleaner that way, both for reading and post-processing.

@maurolacy maurolacy changed the title Add trace functionality Add trace functionality Mar 21, 2023
@maurolacy
Copy link
Contributor Author

Are there plans to merge this? Any feedback on this?

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Sorry, Mauro. I did not look into your two PRs yet.

I think the change makes sense in general. However, given how under-utilized debug is today, I am hesitant to add a new entry point.

What if we make the functionality available in the debug import implementation? I wanted to improve the flexibility of that already. Basically it would be cool if do_debug has a default implementation and also could be improved. println!("{}", msg); is not goo enough for most mid-level cases.

We can add microsecond timestamps in RFC-3339 format my default there:

println!("[2020-12-15T10:57:26.778923Z]: {}", msg);.

@maurolacy
Copy link
Contributor Author

maurolacy commented Apr 19, 2023

Sounds good. I'll do the proposed changes to debug.

Do you think this can be kept always on? The lack of default arguments in Rust makes it difficult to do this in a non-breaking way. In that sense, if we want to be able to enable / disable this functionality, a different entry point is better (less breaking) than adding a parameter to an existing one.

The same with the proposed consumed gas print functionality (#1643).

@maurolacy
Copy link
Contributor Author

Perhaps we can implement a parametrised debug, and then provide some high level wrappers around it?

@webmaster128
Copy link
Member

I'm thinking roughly of a solution like this is MockQuerier:

    /// A handler to handle custom queries. This is set to a dummy handler that
    /// always errors by default. Update it via `with_custom_handler`.
    ///
    /// Use box to avoid the need of another generic type
    custom_handler: Box<dyn for<'a> Fn(&'a C) -> MockQuerierCustomHandlerResult>, 

with a default value

            // strange argument notation suggested as a workaround here: https://github.com/rust-lang/rust/issues/41078#issuecomment-294296365
            custom_handler: Box::from(|_: &_| -> MockQuerierCustomHandlerResult {
                SystemResult::Err(SystemError::UnsupportedRequest {
                    kind: "custom".to_string(),
                })
            }),

that can be overriden:

    pub fn with_custom_handler<CH: 'static>(mut self, handler: CH) -> Self
    where
        CH: Fn(&C) -> MockQuerierCustomHandlerResult,
    {
        self.custom_handler = Box::from(handler);
        self
    }

If we have that available in Environment, we can call it.

With something like Rc instead of Box it should be possible to implement clone.

Ideally the function is optional. If print_debug is false (all production environments), no function is set and called in do_debug. If print_debug is true, we have a default implementation (maybe with the gas value and secod precision time stamps). And users have a way to override it to a custom debug implementation to e.g. write to STDERR instead of STDOUT, add more tracing data or write to file.

@maurolacy
Copy link
Contributor Author

maurolacy commented Apr 20, 2023

Not sure I understand. The original idea / proposal is having debug with timestamps and gas consumption, for real (non-mocked) environments. This allows you to print out timestamps and gas consumption in a node running with --trace. This is useful to measure the performance of wasm code in a real scenario.

Update: Oh, you mean like the custom querier impl, but for debug. Let's discuss in chat how to do this in detail.

@webmaster128
Copy link
Member

Oh, yeah, the like is important here. See #1667

@maurolacy
Copy link
Contributor Author

Closed in favour of #1667.

@maurolacy maurolacy closed this Apr 28, 2023
@webmaster128 webmaster128 deleted the feature/trace branch April 28, 2023 10:09
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.

3 participants