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

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

Merged
merged 6 commits into from
Feb 24, 2024

Conversation

DaniPopes
Copy link
Member

@DaniPopes DaniPopes commented Feb 20, 2024

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:
image

Solmate:
image

sablier/v2-core:

ethereum-optimism/optimism packages/contracts-bedrock
image

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DaniPopes DaniPopes marked this pull request as draft February 20, 2024 23:34
@DaniPopes DaniPopes marked this pull request as ready for review February 21, 2024 00:20
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be fine with that.

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

Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mds1
Copy link
Collaborator

mds1 commented Feb 21, 2024

cc @0xalpharush for thoughts on how this affects slither

@gakonst
Copy link
Member

gakonst commented Feb 21, 2024

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

@DaniPopes
Copy link
Member Author

DaniPopes commented Feb 21, 2024

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
Copy link
Contributor

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

karmacoma-eth added a commit to a16z/halmos that referenced this pull request Mar 1, 2024
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
zeroXbrock pushed a commit to flashbots/suavex-foundry that referenced this pull request Apr 2, 2024
…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
skhomuti added a commit to lidofinance/community-staking-module that referenced this pull request Apr 24, 2024
#151)

…ch is absent by default in the current foundry version

foundry-rs/foundry#7197

Co-authored-by: Dmitry Gusakov <dmitry.gusakov@lido.fi>
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

6 participants