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

v1.0.0 causes compiling with via-ir to take substantially longer #207

Closed
Vectorized opened this issue Nov 3, 2022 · 25 comments · Fixed by #217
Closed

v1.0.0 causes compiling with via-ir to take substantially longer #207

Vectorized opened this issue Nov 3, 2022 · 25 comments · Fixed by #217

Comments

@Vectorized
Copy link

Vectorized commented Nov 3, 2022

Recently, I added a via-ir testing to Solady for more robust testing, since it is a library.

However, with the latest version of forge-std, the CI is still unable to finish compiling the code after 30 minutes.

See this workflow run: https://github.com/Vectorized/solady/actions/runs/3385556435/jobs/5623841631 (not complete even after 30 minutes)

I left it compiling for more than 3 hours on my machine, to no avail too.

Reverting back forge-std to an eariler commit c19dfd2f2a88a461216b0dd1f4961e1a85dcad46 seems to solve the issue.

See this workflow run: https://github.com/Vectorized/solady/actions/runs/3386504801/jobs/5625994122 (completes under 2 minutes)

@Vectorized Vectorized changed the title Compiling with via-ir takes substantially longer v1.0.0 causes compiling with via-ir to take substantially longer Nov 3, 2022
@leonardoalt
Copy link

Does the Solidity version also change between the two tests?

@mds1
Copy link
Collaborator

mds1 commented Nov 3, 2022

It looks like they're both 0.8.17

Successful one:
image

Failed one
image

forge-std was made a lot more modular so the change from 65 to 73 files seems reasonable to me

@emo-eth
Copy link
Contributor

emo-eth commented Nov 3, 2022

I stumbled across this Solidity issue from the Seaport release - I don't fully follow the thread, but it seems like via-ir slowness isn't an issue with other solc compilers, just the one supplied by svm-rs – which means at least part of this slowness is probably due an upstream foundry or svm-rs issue?
ethereum/solidity#13050

@Vectorized
Copy link
Author

Looks like compiling forge-std alone with via-ir takes incredibly long since the v1.0.0 commit.

The previous commit 2a2ce3692b8c1523b29de3ec9d961ee9fbbc43a6, still compiles reasonably fast.

@Vectorized
Copy link
Author

Vectorized commented Nov 3, 2022

Update: looks like commit 72cdd70ae608e32711b4bcf3bbcdafc9eddc3c56 in v0.3 is the start of the insanely long compile times.

#195

I suspect it is due to the _constructor in StdCheatsSafe. And due to vm.rpcUrls().

I changed the constructor into a lazy initializer and the compilation went under a minute.

@mattsse
Copy link
Member

mattsse commented Nov 3, 2022

I don't fully follow the thread, but it seems like via-ir slowness isn't an issue with other solc compilers

Do you mean different solc versions?

what's your OS?

@Vectorized
Copy link
Author

@mattsse @leonardoalt @mds1 See my previous comment.

@mattsse
Copy link
Member

mattsse commented Nov 3, 2022

we're only talking about solc compile times, right?

@mattsse
Copy link
Member

mattsse commented Nov 3, 2022

can you try this locally and check when you notice it (during or after the compile spinner?). I suspect this is actually testing not compiling.

@Vectorized
Copy link
Author

I'm using Ubuntu 20.04.4 LTS.

Both forge build --via-ir and forge test --via-ir takes incredibly long.

@mds1
Copy link
Collaborator

mds1 commented Nov 3, 2022

@Vectorized yea I've confirmed that commenting out the _constructor logic speeds up the compile times. @leonardoalt any idea why that block would cause such a performance impact? I haven't yet dug in to see if there's a specific section of that method that's problematic

@Vectorized wdym by "changed the constructor into a lazy initializer and the compilation went under a minute"? like you moved that into a function that needs to be explicitly called instead of running at construction?

@Vectorized
Copy link
Author

Vectorized commented Nov 3, 2022

@mds1

bool private lazyInitialized;

function lazyInitialize() private {
    if (lazyInitialized) return;
    lazyInitialized = true;
    ...
}

function assumeNoPrecompiles(address addr, uint256 chainId) internal virtual {
    lazyInitialize();
    ...
}

Caveat is you can't use stdChains without calling a getter function.

@Vectorized
Copy link
Author

Imo, the greatly improved compilation time for via-ir CI pipelines is worth the drop in syntactic sugar.

@leonardoalt
Copy link

@mds1 haven't checked at all but could be related to the optimizer since there's some struct stuff. Can only check tomorrow tho (it's evening here now)

@leonardoalt
Copy link

leonardoalt commented Nov 3, 2022

Ok definitely something weird with solc:

Inside forge-std, running the command below finishes in one second.

solc --base-path . --bin --via-ir --optimize src/Test.sol

Now create a file src/a.sol:

import "./Test.sol";
contract C is Test {}

and run

solc --base-path . --bin --via-ir --optimize src/a.sol

this now hangs.

@leonardoalt
Copy link

Ok that's just because Test is abstract though. If it's not abstract it takes 40s, but does finish.

@mds1
Copy link
Collaborator

mds1 commented Nov 3, 2022

So if I build via forge, it takes ~900 seconds. If I run solc --base-path . --bin --via-ir --optimize src/Test.sol it takes 88 seconds... 🤔

@emo-eth
Copy link
Contributor

emo-eth commented Nov 4, 2022

I don't fully follow the thread, but it seems like via-ir slowness isn't an issue with other solc compilers

Do you mean different solc versions?

what's your OS?

I mean solc binary distributions - from issue I linked issue, @hrkrshnn pointed out that the Rust solc binaries that svm-rs uses have debug symbols included – it sounds like that's a drag on performance.

This specific issue might still be a separate edge case, though.

Edit: Yeah, ran through the steps in that thread and it seems debug symbols are no longer present - you can ignore me!

@leonardoalt
Copy link

So if I build via forge, it takes ~900 seconds. If I run solc --base-path . --bin --via-ir --optimize src/Test.sol it takes 88 seconds... 🤔

Yea, also have that. Maybe forge is just trying to compile other stuff too that always get similarly stuck? I'm also trying this only on forge-std, it's enough to replicate.

@mds1
Copy link
Collaborator

mds1 commented Nov 4, 2022

@mattsse is there a way to only compile a single contract with forge build? You could use the --skip flag and skip everything else but that seems tedious. But I guess this means the two possible sources of error are:

  1. Something in forge
  2. The solc binaries used/built by svm-rs differ somehow, since I think for some versions @roynalnaruto builds the binaries?

And it seems we can rule out an underlying solc error given the speed when invoking solc directly

@mds1
Copy link
Collaborator

mds1 commented Nov 7, 2022

@roynalnaruto Are all svm-rs binaries directly from https://binaries.soliditylang.org/ or are some custom compiled?

@roynalnaruto
Copy link

roynalnaruto commented Nov 8, 2022

Binaries for Mac M1 (macos:aarch64) and Linux Aarch64 (linux:aarch64) are custom compiled, more info in this repo and this repo respectively.

For other targets, i.e. linux:amd64, macos:amd64 and windows:amd64, they're fetched from binaries.soliditylang.

@mds1
Copy link
Collaborator

mds1 commented Nov 8, 2022

Ah I'm macos:amd64 so seems this isn't an issue with svm-rs builds then, thanks 🙂

@mattsse
Copy link
Member

mattsse commented Nov 8, 2022

I wasn't able to reproduce this on solady master (the branch that belongs to the linked CI run is deleted so assuming it was merged)

FOUNDRY_PROFILE=via-ir forge test --force                                                            
[⠊] Compiling...
[⠆] Compiling 65 files with 0.8.17
[⠃] Solc 0.8.17 finished in 154.27s
Compiler run successful

but was able to reproduce the behavior @leonardoalt described, with solc 0.8.17 on intel mac.
and tracked this down to StdCheatsSafe

And it seems we can rule out an underlying solc error given the speed when invoking solc directly

since this happens when invoking solc directly, I don't think this is correct.

I thought we already identified the _constructor() call as the root cause?

So if I build via forge, it takes ~900 seconds. If I run solc --base-path . --bin --via-ir --optimize src/Test.sol it takes 88 seconds... 🤔

Test is an abstract contract, so perhaps this plays a role here, because

solc --base-path . --bin --via-ir --optimize src/a.sol

takes significantly longer for a.sol:

import "./Test.sol";
contract C is Test {}

@leonardoalt
Copy link

Yea exactly, you need to make Test non abstract to make it take long. This seems to be some edge case in the optimizer, someone from Solidity is already taking a look.

karmacoma-eth added a commit to showtime-xyz/showtime-contracts that referenced this issue Nov 15, 2022
Beware this issue wrt slow build times: foundry-rs/forge-std#207
karmacoma-eth added a commit to showtime-xyz/showtime-contracts that referenced this issue Dec 9, 2022
Beware this issue wrt slow build times: foundry-rs/forge-std#207
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 a pull request may close this issue.

6 participants