-
Notifications
You must be signed in to change notification settings - Fork 341
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
Compilation still slow with via-ir #225
Comments
Just want to confirm my understanding and make sure we're on the same page:
There was speculation in #207 that the slow via-ir build times might be related to something in foundry/svm-rs, as opposed to generic solc slowness, which is why I want to confirm that hardhat's 16s time is not including the foundry scripts/tests. @marktoda Do you mind trying a few other benchmarks and sharing the results? (I tested these changes in forge-std directly using the
If you see similar results, there's two options to consider.
I'd be curious to hear your thoughts on each. I think the solc team is working on improving via-ir build times too, so hopefully this becomes less of an issue in future versions, but it makes sense the optimizer will always be slower than no optimizer |
@marktoda WDYT re Matt's response above? |
Thanks for the analysis here! running these now:
I think having a test profile with no optimizer steps is a good option, hadn't considered that before. A bit of a hassle / chance of forgetting to set to release mode and accidentally releasing unoptimized code though IMO the lazy initialization is worth it for a 2x speedup, though I'm not sure I fully understand the UX issue |
Oh, other problem with the test/release profiles is that we heavily weigh gas snapshots in PR reviews. These snapshots would now be much less reflective of real gas if not using optimizer when generating them |
Not a big one, it's just instead of calling |
Ok great. So let’s move to lazy initialization and document the optimizer suggestion in the book as well. I'm traveling this week, @ZeroEkkusu do you think you can take the lead on making these changes?
Agreed, the way I like to handle this is keep the default profile to have the release settings, and use that in CI, then a lite profile for local dev and testing. I think this makes it less likely to forget to set release mode, since you would need to explicitly enable the wrong profile during deployment. Regarding snapshots, @gakonst maybe we bump the priority of foundry-rs/foundry#2056 and discuss options/UX there, and make them easier to integrate into CI? |
Haven't thought about snapshots at all, let's move conversation there. @ZeroEkkusu if you can take point in the forge-std optimizations that would be great. And we can follow-up with the release mode discussion in a separate issue, if @mds1 can take point there with a braindump. |
@gakonst re lite vs. release mode, just created foundry-rs/book#709 |
Im on forge-std 1.5 and compile times on a rather simple code base are still over 35 seconds. optimizer = true [⠔] Compiling 63 files with 0.8.17 |
@zeroknots Can you open a new issue with steps to reproduce? And can you benchmark the time to only compile the source files, compared to both source + test, to make sure the slowness is caused by forge-std? |
Done :) Hope this is helpful to triage. |
Was chatting with Georgios about this and ran some benchmarks / tests on https://github.com/uniswap/universal-router. Posting here to hopefully help debug this
Repo:
Cold build with hardhat: 16s
Cold build with forge: 45s
Removing only test/: 23s
Removing only script/: 30s
Removing test/ and script/: 7s
Removing forge-std and test/ and script: 6.5s
With the hunch that console.log or console2.log was causing this, i removed those from forge-std and all callsites: 40s
So, something in forge-std is causing the slow compilation and it's not console. I can maybe go one-by-one and test further later but wanted to post here in case other folks have time to TAL. 45s obviously not terrible but 3x worse than hardhat and bit of a drag on development
The text was updated successfully, but these errors were encountered: