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

Stack shuffling refactor #14851

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft

Stack shuffling refactor #14851

wants to merge 3 commits into from

Conversation

r0qs
Copy link
Member

@r0qs r0qs commented Feb 13, 2024

No description provided.

@r0qs r0qs force-pushed the stackShufflingPerformance branch 5 times, most recently from 4102ad6 to 5f291e4 Compare February 13, 2024 15:47
@r0qs r0qs self-assigned this Feb 19, 2024
@r0qs r0qs force-pushed the stackShufflingPerformance branch 2 times, most recently from c8aceb0 to cc502d2 Compare February 19, 2024 13:57
@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Mar 5, 2024
@ethereum ethereum deleted a comment from github-actions bot Mar 5, 2024
@r0qs r0qs removed the stale The issue/PR was marked as stale because it has been open for too long. label Mar 5, 2024
@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Mar 20, 2024
@ethereum ethereum deleted a comment from github-actions bot Mar 25, 2024
@r0qs r0qs removed the stale The issue/PR was marked as stale because it has been open for too long. label Mar 25, 2024
@r0qs r0qs force-pushed the stackShufflingPerformance branch from cc502d2 to 700c5fa Compare March 25, 2024 12:01
@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Apr 8, 2024
@cameel cameel removed the stale The issue/PR was marked as stale because it has been open for too long. label Apr 8, 2024
@r0qs r0qs force-pushed the stackShufflingPerformance branch from 700c5fa to b93d4ac Compare April 22, 2024 14:09
@ethereum ethereum deleted a comment from github-actions bot Apr 22, 2024
@r0qs r0qs force-pushed the stackShufflingPerformance branch from b93d4ac to aba9a72 Compare April 22, 2024 14:21
@r0qs
Copy link
Member Author

r0qs commented Apr 22, 2024

Benchmarks results:

File Pipeline Bytecode size Time Exit code
verifier.sol legacy 4874 bytes 0.13 s 0
verifier.sol via-ir 4351 bytes 0.59 s 0
OptimizorClub.sol legacy 0 bytes 0.46 s 1
OptimizorClub.sol via-ir 22193 bytes 3.38 s 0
chains.sol legacy 5845 bytes 0.17 s 0
chains.sol via-ir 23043 bytes 15.12 s 0
  • This branch:
File Pipeline Bytecode size Time Exit code
verifier.sol legacy 4874 bytes 0.14 s 0
verifier.sol via-ir 4351 bytes 0.58 s 0
OptimizorClub.sol legacy 0 bytes 0.47 s 1
OptimizorClub.sol via-ir 22193 bytes 3.33 s 0
chains.sol legacy 5845 bytes 0.17 s 0
chains.sol via-ir 23043 bytes 12.15 s 0

@cameel
Copy link
Member

cameel commented Apr 22, 2024

So only chains.sol is affected, just like in #14112? Odd. We need more benchmark contracts.

@cameel
Copy link
Member

cameel commented Apr 22, 2024

It would be interesting to see it benchmarked the way I did in #14854 (comment)

This would show pure compilation time on some actual projects (I chose Uniswap v4 and Seaport since their latest versions compile without any workarounds) but with less variance than we see in CI. Or at least in a way that makes it easier to run multiple times and average the results.

@cameel
Copy link
Member

cameel commented Apr 22, 2024

On the topic of the PR itself - I tried to review it but the diff is almost useless due to all the moved code. It also has no description or changelog and commit descriptions are not very informative, so I'm not really sure what to expect here. The title sounds as if you were just making the code nicer but it does not seem to be all that you're doing here. If you could separate the refactors from actual functional changes, it would be much easier to review.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label May 7, 2024
@ethereum ethereum deleted a comment from github-actions bot May 7, 2024
@r0qs r0qs removed the stale The issue/PR was marked as stale because it has been open for too long. label May 7, 2024
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.

None yet

3 participants