feat: don't request Solc JSON AST unless absolutely necessary #7197

DaniPopes · 2024-02-20T22:27:04Z

The Solc JSON AST output is very expensive to both serialize on the compiler's side, and deserialize on our side due to its very nested nature and generally big size.

This is especially noticeable on hot starts thanks to less time spent deserializing all the already-compiled artifacts. This is applicable to all commands that require a built project.

Adds the ast = <bool> config and --ast flag to build args to optionally include the AST in the artifacts.

We currently use the AST in:

coverage
flatten
inline test config (NatSpec)

coverage and flatten have been updated to override the ast config.
test has not been updated for inline test config since it currently does not work, and all tests pass without it. It will have to be rewritten using solang_parser or something else.

Results

Note that I'm using the debug profile to build forge, but this won't matter much as most of the time is spent in solc anyway.

Default forge init project:

Solmate:

sablier/v2-core:

out/ size before: 144Mb
out/ size after: 106Mb
profile before: https://share.firefox.dev/42KyHyM
profile after: https://share.firefox.dev/3T2RrpQ

ethereum-optimism/optimism packages/contracts-bedrock

mattsse

does this affect the regular forge build artifacts?

~~we added metadata because this is apparently used by some tools~~

that's not part of metadata, so I think we can do that, but not entirely sure

mattsse

I'd be fine with that.

we should check with slither first if they're relying on this in the artifacts file

onbjerg

some external tools rely on this, so i'd add a flag to forge build to get this back

mds1 · 2024-02-21T00:41:07Z

cc @0xalpharush for thoughts on how this affects slither

gakonst · 2024-02-21T00:51:10Z

Can we get a comparison for release mode compilation time improvements due to this on a large repo? e.g. Optimism contracts

crates/forge/bin/cmd/coverage.rs

DaniPopes · 2024-02-21T01:48:35Z

Can we get a comparison for release mode compilation time improvements due to this on a large repo? e.g. Optimism contracts

@gakonst
The overwhelming majority of the time is spent in solc so release mode on our side won't do much here.

It's 2-3% faster on clean builds, up to 2-3x for sablier/v2-core (absolute time save of about 2.5s for both). Note that time saved is proportional to amount of total artifacts.
Same for optimism contracts (updated description).

0xalpharush · 2024-02-21T03:24:16Z

I ran foundryup -b dani/drop-ast and it does make slither not work since the ASTs are missing. I think it is fine to drop from forge build but I'd prefer forge build --build-info (the hardhat artifact style) to be backwards compatible ofc. Otherwise, we'd have to cut a release and handle the case where people are not on this version of foundry yet

Forge has recently (foundry-rs/foundry#7197) dropped the ast from the build json by default, so we need to request it explicitly with `--ast`, `ast = <bool>` config or just `--build-info` to avoid checking if forge supports the `--ast` command

…y-rs#7197) * feat: don't request Solc JSON AST unless absolutely necessary * fix: don't require AST just for the path of a file * feat: add `--abi` and `abi = <bool>` config values * fmt * fix config * fix: keep AST in build_info

#151) …ch is absent by default in the current foundry version foundry-rs/foundry#7197 Co-authored-by: Dmitry Gusakov <dmitry.gusakov@lido.fi>

feat: don't request Solc JSON AST unless absolutely necessary

dc09f4a

DaniPopes requested review from Evalir and mattsse as code owners February 20, 2024 22:27

fix: don't require AST just for the path of a file

96a3496

mattsse reviewed Feb 20, 2024

View reviewed changes

DaniPopes marked this pull request as draft February 20, 2024 23:34

DaniPopes marked this pull request as ready for review February 21, 2024 00:20

DaniPopes requested review from mattsse and klkvr February 21, 2024 00:23

mattsse reviewed Feb 21, 2024

View reviewed changes

onbjerg requested changes Feb 21, 2024

View reviewed changes

feat: add --abi and abi = <bool> config values

c62b846

DaniPopes requested a review from onbjerg February 21, 2024 01:04

onbjerg approved these changes Feb 21, 2024

View reviewed changes

onbjerg reviewed Feb 21, 2024

View reviewed changes

crates/forge/bin/cmd/coverage.rs Outdated Show resolved Hide resolved

DaniPopes added 2 commits February 21, 2024 02:11

fmt

359e804

fix config

99bbfbf

fix: keep AST in build_info

b713638

DaniPopes merged commit 81af7d3 into master Feb 24, 2024
20 checks passed

DaniPopes deleted the dani/drop-ast branch February 24, 2024 03:06

saeta-eth mentioned this pull request Feb 27, 2024

fix(cli): Check forge ast support usecannon/cannon#792

Merged

anvacaru mentioned this pull request Feb 28, 2024

[Hotfix] Add support for latest foundry version runtimeverification/kontrol#399

Merged

palinatolmach mentioned this pull request Feb 28, 2024

Use --ast in forge build in Kontrol runtimeverification/kontrol#400

Closed

This was referenced Feb 28, 2024

(Need help) AST parser fails due to missing "ast" field Cyfrin/aderyn#191

Closed

v0.0.15 Staging Cyfrin/aderyn#176

Merged

DaniPopes mentioned this pull request Feb 29, 2024

fix: use ArtifactId.source instead of .path #7271

Merged

0xkarmacoma mentioned this pull request Mar 1, 2024

Add --build-info to forge build command a16z/halmos#255

Merged

grandizzy mentioned this pull request Mar 5, 2024

In-line test configuration not respected #7310

Closed

2 tasks

DaniPopes mentioned this pull request Mar 5, 2024

fix inline test config: enable ast for test #7314

Closed

adamegyed mentioned this pull request Mar 6, 2024

Document the --ast flag as an option for forge build foundry-rs/book#1134

Closed

yonadaa mentioned this pull request Mar 7, 2024

feat(cli): link and deploy public libraries latticexyz/mud#1910

Merged

yi-sun mentioned this pull request Mar 13, 2024

fix: set ast = true in foundry.toml axiom-crypto/axiom-std#12

Merged

mds1 mentioned this pull request Mar 25, 2024

contracts: More robust abi/storage snapshot gen ethereum-optimism/optimism#9950

Merged

TilakMaddy mentioned this pull request Mar 29, 2024

error: unexpected argument '--ast' found Cyfrin/aderyn#265

Closed

skhomuti mentioned this pull request Apr 24, 2024

fix: abis requires ast keys to be present in the build outputs, whi… lidofinance/community-staking-module#151

Merged

This was referenced Apr 26, 2024

Reset to upstream phylaxsystems/phoundry#12

Merged

feat/forge bin as lib phylaxsystems/phoundry#13

Merged

0xkarmacoma mentioned this pull request May 7, 2024

foundry build cache is disabled with --build-info (it always rebuilds) a16z/halmos#285

Closed

saeta-eth mentioned this pull request Oct 16, 2024

feat(cli): forge build for tests by default usecannon/cannon#1473

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: don't request Solc JSON AST unless absolutely necessary #7197

feat: don't request Solc JSON AST unless absolutely necessary #7197

DaniPopes commented Feb 20, 2024 •

edited

Loading

mattsse left a comment •

edited

Loading

mattsse left a comment

onbjerg left a comment

mds1 commented Feb 21, 2024

gakonst commented Feb 21, 2024

DaniPopes commented Feb 21, 2024 •

edited

Loading

0xalpharush commented Feb 21, 2024

feat: don't request Solc JSON AST unless absolutely necessary #7197

feat: don't request Solc JSON AST unless absolutely necessary #7197

Conversation

DaniPopes commented Feb 20, 2024 • edited Loading

Results

mattsse left a comment • edited Loading

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

onbjerg left a comment

Choose a reason for hiding this comment

mds1 commented Feb 21, 2024

gakonst commented Feb 21, 2024

DaniPopes commented Feb 21, 2024 • edited Loading

0xalpharush commented Feb 21, 2024

DaniPopes commented Feb 20, 2024 •

edited

Loading

mattsse left a comment •

edited

Loading

DaniPopes commented Feb 21, 2024 •

edited

Loading