forge-std icon indicating copy to clipboard operation
forge-std copied to clipboard

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

Open Vectorized opened this issue 2 years ago • 19 comments

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 avatar Nov 03 '22 14:11 Vectorized

Does the Solidity version also change between the two tests?

leonardoalt avatar Nov 03 '22 14:11 leonardoalt

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

mds1 avatar Nov 03 '22 14:11 mds1

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? https://github.com/ethereum/solidity/issues/13050

emo-eth avatar Nov 03 '22 15:11 emo-eth

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 avatar Nov 03 '22 17:11 Vectorized

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

https://github.com/foundry-rs/forge-std/pull/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.

Vectorized avatar Nov 03 '22 18:11 Vectorized

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?

mattsse avatar Nov 03 '22 18:11 mattsse

@mattsse @leonardoalt @mds1 See my previous comment.

Vectorized avatar Nov 03 '22 18:11 Vectorized

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

mattsse avatar Nov 03 '22 18:11 mattsse

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.

mattsse avatar Nov 03 '22 18:11 mattsse

I'm using Ubuntu 20.04.4 LTS.

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

Vectorized avatar Nov 03 '22 18:11 Vectorized

@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?

mds1 avatar Nov 03 '22 18:11 mds1

@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 avatar Nov 03 '22 18:11 Vectorized

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

Vectorized avatar Nov 03 '22 18:11 Vectorized

@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 avatar Nov 03 '22 19:11 leonardoalt

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 avatar Nov 03 '22 20:11 leonardoalt

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

leonardoalt avatar Nov 03 '22 20:11 leonardoalt

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... 🤔

mds1 avatar Nov 03 '22 23:11 mds1

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!

emo-eth avatar Nov 04 '22 00:11 emo-eth

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.

leonardoalt avatar Nov 04 '22 07:11 leonardoalt

@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 avatar Nov 04 '22 18:11 mds1

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

mds1 avatar Nov 07 '22 14:11 mds1

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.

roynalnaruto avatar Nov 08 '22 01:11 roynalnaruto

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

mds1 avatar Nov 08 '22 01:11 mds1

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 {}

mattsse avatar Nov 08 '22 08:11 mattsse

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.

leonardoalt avatar Nov 08 '22 09:11 leonardoalt