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

dryrun time consuming profiling #2

Merged
merged 3 commits into from
Sep 26, 2023
Merged

dryrun time consuming profiling #2

merged 3 commits into from
Sep 26, 2023

Conversation

dajuguan
Copy link

fix dry_run time consuming bug, which is caused by an O(n) operation in pop private_inputs

Copy link

@LiuJiazheng LiuJiazheng left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -82,7 +82,9 @@ pub fn register_wasm_input_foreign(
context.push_public(value);
value
} else {
context.pop_private()
let value = context.pop_private();
println!("wasm_output:{:?}", value);
Copy link

Choose a reason for hiding this comment

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

remove this line

Copy link
Author

Choose a reason for hiding this comment

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

removed in 326e299

@@ -43,7 +43,7 @@ impl Context {
if self.private_inputs.is_empty() {
panic!("failed to read private input, please checkout your input");
}
self.private_inputs.remove(0)
self.private_inputs.pop().unwrap()
Copy link

Choose a reason for hiding this comment

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

I guess this will break --private argument. I am not a big fan of reverse as it results in a different order than other inputs such as context_in, etc - introducing additional context for maintainer. I think a better fix is to use VecDeque.pop_front()

https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.pop_front

or you can return data.into_iter() and get the data via iter.next()

Copy link
Author

Choose a reason for hiding this comment

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

use VecDeque instead in 326e299

@dajuguan dajuguan merged commit 0b0ded4 into dev Sep 26, 2023
1 check failed
@dajuguan dajuguan deleted the dryrun_profiling branch November 15, 2023 09:18
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