-
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
Move benchmark contracts to era_vm #218
Conversation
Ergs comparison results:
|
@@ -84,6 +84,9 @@ bench-setup: submodules build_bench_contracts $(ZKSYNC_BENCH_TEST_DATA) $(ZKSYNC | |||
bench: | |||
cd $(ZKSYNC_ROOT) && cargo bench --bench criterion "$(lambda|fast_vm|legacy)/(fibonacci|send)" | |||
|
|||
check-flamegraph: | |||
cd $(ZKSYNC_ROOT) && CARGO_PROFILE_BENCH_DEBUG=2 cargo flamegraph --root --bench criterion -- --bench --profile-time 5 "$lambda/fibonacci_rec^" |
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 the $lambda
is converted to ambda
for some reason. What does $l
do here? Is the $
even necessary?
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.
No, it is not necessary actually. I had been using it without any problems though, so I think it works ok.
def parse_benchmark_file(file_path): | ||
machines = {} | ||
pattern = r'(\w+)/\w+\s+time:\s+\[\d+(?:\.\d+)?\s(\w+)\s(\d+(?:\.\d+)?)\s(\w+)\s\d+(?:\.\d+)?\s(\w+)\]' | ||
|
||
with open(file_path, 'r') as file: | ||
content = file.read() | ||
# Clean the file content | ||
cleaned_content = re.sub(r'\n(\s+)time:', ' time:', content) | ||
for line in cleaned_content.split('\n'): | ||
match = re.match(pattern, line) | ||
if match: | ||
machine, lower_unit, mid_time, mid_unit, upper_unit = match.groups() | ||
# Convert mid_time to milliseconds | ||
mid_time_ms = convert_to_ms(float(mid_time), mid_unit) | ||
machines[machine] = machines.get(machine, 0) + mid_time_ms | ||
|
||
return machines |
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.
criterion
is capable of emitting JSON IIRC. Maybe that could save you parsing.
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 didn't know that, it might be useful to improve the script. Thanks!
Just to make sure, we should check the generated assembly for the contracts to confirm they do what we think they do. |
LGTM |
This PR moves custom made benchmark contracts sources from zksync-era to era_vm, so its easier to modify them.
It also adds a new benchmark result processing script made in python, which allows us to easily aggregate all benchmark execution times.