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

Performance regression for invariant tests #7503

Closed
2 tasks done
frontier159 opened this issue Mar 26, 2024 · 8 comments · Fixed by #7505
Closed
2 tasks done

Performance regression for invariant tests #7503

frontier159 opened this issue Mar 26, 2024 · 8 comments · Fixed by #7505
Labels
T-bug Type: bug

Comments

@frontier159
Copy link
Contributor

frontier159 commented Mar 26, 2024

Component

Forge

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (563e062 2024-03-26T00:16:13.950236000Z)

What command(s) is the bug in?

forge test

Operating System

macOS (Apple Silicon)

Describe the bug

A performance regression has been introduced to invariant tests - they've slowed down considerably with a recent foundryup.

Using a version from early March:

> foundryup --version nightly-de33b6af53005037b463318d2628b5cfcaf39916
> time forge test --mt invariant --offline
...
Suite result: ok. 5 passed; 0 failed; 0 skipped; finished in 13.90s (63.18s CPU time)

Ran 1 test suite in 13.91s (13.90s CPU time): 5 tests passed, 0 failed, 0 skipped (5 total tests)
forge test --mt invariant --offline  62.56s user 0.56s system 431% cpu 14.636 total

Using latest:

> foundryup # latest
> time forge test --mt invariant --offline
...
Suite result: ok. 5 passed; 0 failed; 0 skipped; finished in 80.10s (344.30s CPU time)

Ran 1 test suite in 80.12s (80.10s CPU time): 5 tests passed, 0 failed, 0 skipped (5 total tests)
forge test --mt invariant --offline  327.82s user 2.22s system 409% cpu 1:20.62 total
@frontier159 frontier159 added the T-bug Type: bug label Mar 26, 2024
@klkvr
Copy link
Member

klkvr commented Mar 27, 2024

Do you have any handlers using vm.assume? #7309 could've resulted in longer runs for such suites which is actually just happening because deeper individual runs are done more often

@frontier159
Copy link
Contributor Author

Do you have any handlers using vm.assume? #7309 could've resulted in longer runs for such suites which is actually just happening because deeper individual runs are done more often

I don't use vm.assume, I also don't think I use any of the StdCheats which call that either.

Unfortunately a private repo, would be tricky/time consiuming to try and repro it. Any other repos like sablier showing the symptoms?

@frontier159
Copy link
Contributor Author

frontier159 commented Mar 27, 2024

Actually @klkvr you can reproduce in a slightly older public snapshot of the repo:
https://github.com/TempleDAO/origami-public/tree/main/apps/protocol/

  1. clone with submodules
  2. nvm use
  3. yarn
  4. time forge test --mt invariant

Will also need $MAINNET_RPC_URL set

@grandizzy
Copy link
Collaborator

grandizzy commented Mar 27, 2024

had a look at, looks like hashbrown performance better than std's is due to the introduction of indexset for state (in #7395)

Ran 1 test suite in 21.24s (21.22s CPU time): 5 tests passed, 0 failed, 0 skipped (5 total tests)

vs

Ran 1 test suite in 81.53s (81.51s CPU time): 5 tests passed, 0 failed, 0 skipped (5 total tests)

CC @DaniPopes or could be the introduction of indexset for state (instead regular hashmap) wdyt? any thoughts if this could be improved somehow?

LE: it looks like using std BTreeSet instead IndexSet yields better performance....

Ran 1 test suite in 27.33s (27.30s CPU time): 5 tests passed, 0 failed, 0 skipped (5 total tests)

@DaniPopes
Copy link
Member

DaniPopes commented Mar 27, 2024

hm yeah we do try to insert the entire EVM stack on step. I believe it's fxhash, I hadn't considered fxhash was this slow for 32-byte values, @grandizzy can you try by just removing BuildHasherDefault<fxhash> in the type alias so it defaults to the default std hasher?

@grandizzy
Copy link
Collaborator

grandizzy commented Mar 27, 2024

BuildHasherDefault

@DaniPopes yep, much better (even than original run)

Ran 1 test suite in 19.11s (19.08s CPU time): 5 tests passed, 0 failed, 0 skipped (5 total tests)

want me to do a PR or will you?

@DaniPopes
Copy link
Member

Go ahead

@frontier159
Copy link
Contributor Author

Fantastic I'll check on the next nightly thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants