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

Via IR bytecode comparison #14355

Merged
merged 4 commits into from
Jul 14, 2023
Merged

Via IR bytecode comparison #14355

merged 4 commits into from
Jul 14, 2023

Conversation

cameel
Copy link
Member

@cameel cameel commented Jun 23, 2023

Depends on #14354. Merged.
Depends on #14374.
Fixes #13583.

This is the final PR for --via-ir bytecode comparison.

@cameel cameel added testing 🔨 has dependencies The PR depends on other PRs that must be merged first labels Jun 23, 2023
@cameel cameel self-assigned this Jun 23, 2023
@@ -88,6 +88,7 @@ commands:
steps:
- run:
name: Generate bytecode reports for the selected preset
no_output_timeout: 30m
Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately even after all that upfront work to get things working in parallel, it's pretty slow. Optimized via IR preset takes as long as 25 min on windows and on emscripten.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What does no_output_timeout do? Fail the step if the runtime exceeds 30 minutes? If so, then a 5 minute buffer for a job that takes 25 minutes is a little tight, no? Would it make sense to bump this a little bit; maybe 35 minutes?

Copy link
Member Author

@cameel cameel Jul 14, 2023

Choose a reason for hiding this comment

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

Yeah. But the default timeout is 10 min so I'm increasing it, not decreasing it :)

I think 30 min is fine. If it fails we can still increase it but we'll get a clear signal that things are getting out of hand. 25 min is actually way more than I expected. I'd prefer this to finish much sooner. If not for the fact that we're working on IR performance already, I'd suggest parallelizing things even more to improve it, rather than keep bumping the limit.

@cameel
Copy link
Member Author

cameel commented Jun 23, 2023

Timing, from the most recent run:

Job Preset Legacy Via IR
b_bytecode_win optimize 6m 52s 24m 24s
b_bytecode_win no-optimize 5m 5s 10m 12s
b_bytecode_osx optimize 3m 56s 10m 42s
b_bytecode_osx no-optimize 3m 38s 5m 55s
b_bytecode_ems optimize 5m 31s 26m 47s
b_bytecode_ems no-optimize 2m 8s 13m 25s
b_bytecode_ubu optimize 3m 48s 9m 18s
b_bytecode_ubu no-optimize 3m 2s 5m 9s
b_bytecode_ubu_static optimize 2m 12s 7m 51s
b_bytecode_ubu_static no-optimize 1m 13s 3m 35s

@cameel
Copy link
Member Author

cameel commented Jun 23, 2023

Also, we have a problem: t_bytecode_compare does not pass via IR. And it looks like it's due to actual differences in generated bytecode. Weirdly, this happens only for unoptimized code. Optimized is fine.

I will investigate that on Monday.

@cameel cameel force-pushed the parallelize-bytecode-compare-presets branch from 8a3963c to 92816eb Compare June 26, 2023 08:53
Base automatically changed from parallelize-bytecode-compare-presets to develop June 26, 2023 10:57
@cameel
Copy link
Member Author

cameel commented Jun 26, 2023

Current status: blocked by #14361, which I'll be fixing.

If need be, I could add a temporary workaround to delete test cases that do not pass and merge this. I would then remove that workaround in the PR fixing #14361.

@cameel cameel removed the has dependencies The PR depends on other PRs that must be merged first label Jun 26, 2023
@cameel cameel force-pushed the via-ir-bytecode-comparison branch 2 times, most recently from f76c1d3 to 2554a58 Compare June 28, 2023 12:57
@cameel cameel added the has dependencies The PR depends on other PRs that must be merged first label Jun 28, 2023
@cameel cameel force-pushed the via-ir-bytecode-comparison branch 3 times, most recently from 6d7a1fd to d3884ee Compare June 28, 2023 17:29
@cameel
Copy link
Member Author

cameel commented Jun 28, 2023

With #14374 and one additional tweak the bytecode comparison finally passes.

The tweak is basically that one of the tests triggers a YulException (actually stack too deep, but for some reason it's not StackTooDeep) and CLI just barfs while Standard JSON can still successfully return some outputs like metadata. This is basically a hack to ignore those outputs. The proper way to deal with it would be to fix #13925.

The PR is now reviewable but I'll keep it as a draft until #14374 is merged because PRs with dependencies always confuse people :)

matheusaaguiar
matheusaaguiar previously approved these changes Jul 12, 2023
Copy link
Collaborator

@matheusaaguiar matheusaaguiar left a comment

Choose a reason for hiding this comment

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

LGTM

@cameel cameel force-pushed the via-ir-bytecode-comparison branch from d3884ee to b0a99fd Compare July 14, 2023 10:16
@cameel cameel marked this pull request as ready for review July 14, 2023 10:16
@cameel
Copy link
Member Author

cameel commented Jul 14, 2023

Base PR was merged. This is now reviewable.

@matheusaaguiar But since the PR is simple and you already reviewed and approved it, we can also just merge if you reapprove.

Comment on lines +47 to +48
# FIXME: These cases crash because of https://github.com/ethereum/solidity/issues/13583
rm ./*_bytecode_too_large_*.sol ./*_combined_too_large_*.sol
Copy link
Member Author

Choose a reason for hiding this comment

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

@ekpyron dropped #13496 from the focus board recently so I guess he does not want use to focus on fixing in this. For that reason it'll stay like this for the time being. I.e. we're not really running the comparison on all test cases from the repo.

@cameel cameel force-pushed the via-ir-bytecode-comparison branch from b0a99fd to bccfcd4 Compare July 14, 2023 13:28
@@ -88,6 +88,7 @@ commands:
steps:
- run:
name: Generate bytecode reports for the selected preset
no_output_timeout: 30m
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does no_output_timeout do? Fail the step if the runtime exceeds 30 minutes? If so, then a 5 minute buffer for a job that takes 25 minutes is a little tight, no? Would it make sense to bump this a little bit; maybe 35 minutes?

@cameel cameel merged commit e70e595 into develop Jul 14, 2023
1 check passed
@cameel cameel deleted the via-ir-bytecode-comparison branch July 14, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has dependencies The PR depends on other PRs that must be merged first testing 🔨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bytecode compare runs with --via-ir
3 participants