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

Lower integration tests memory usage #327

Merged
merged 8 commits into from
Nov 28, 2024

Conversation

cabrador
Copy link
Collaborator

@cabrador cabrador commented Nov 22, 2024

This PR lowers memory usage for fake net and integration tests by ~500 MB per test.
Before:
Screenshot 2024-11-28 at 12 36 47

After:
Screenshot 2024-11-28 at 12 37 05

part of https://github.com/Fantom-foundation/sonic-admin/issues/50

LuisPH3
LuisPH3 previously approved these changes Nov 22, 2024
cmd/sonictool/db/dbutils.go Outdated Show resolved Hide resolved
cmd/sonicd/app/launcher.go Outdated Show resolved Hide resolved
Copy link
Contributor

@LuisPH3 LuisPH3 left a comment

Choose a reason for hiding this comment

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

Unfortunately, the mechanism used to forward the cache settings into the algorithms that need it is not ok.

func MakeGossipDb(ctx *cli.Context, dbs kvdb.FullDBProducer, dataDir string, validatorMode bool, cacheRatio cachescale.Func) (*gossip.Store, error) {

This function already includes parameters deduced from the command line, additionally this is too late to parse command line arguments, ranges are not checked, and faulty values will be propagated to internal components for undefined behavior.

Please perform the command line arguments parsing the outermost possible layer of the program, sanitize arguments and return an error to the user as early as possible.

To avoid aliasing arguments into the program logic functions (like func x( bool, int , float ,int) I propose alternatives:

  1. pack all configuration arguments from a function into a struct (this way you can add names to the funciton call side)
callFunction(configuration{
     argument1: 1,
     argument2: 2,
     })
  1. Pack single arguments into a struct (same effect, naming one argument at the time).

  2. Use a builder pattern (may be a little too heavy for this use case).

config/flags/flags.go Outdated Show resolved Hide resolved
config/flags/flags.go Outdated Show resolved Hide resolved
tests/integration_test_net.go Show resolved Hide resolved
tests/integration_test_net.go Show resolved Hide resolved
cmd/sonictool/db/dbutils.go Outdated Show resolved Hide resolved
cmd/sonictool/db/dbutils.go Show resolved Hide resolved
@cabrador cabrador force-pushed the c/lower-integration-tests-memory branch 3 times, most recently from 0930f2c to e64997f Compare November 26, 2024 12:05
@cabrador cabrador requested a review from LuisPH3 November 26, 2024 12:09
LuisPH3
LuisPH3 previously approved these changes Nov 26, 2024
cmd/sonictool/db/dbutils.go Outdated Show resolved Hide resolved
config/flags/flags.go Show resolved Hide resolved
tests/integration_test_net.go Outdated Show resolved Hide resolved
tests/integration_test_net.go Show resolved Hide resolved
cmd/sonictool/chain/export_events.go Outdated Show resolved Hide resolved
@cabrador cabrador force-pushed the c/lower-integration-tests-memory branch from d4be9bf to e65e7cc Compare November 28, 2024 10:40
HerbertJordan
HerbertJordan previously approved these changes Nov 28, 2024
cmd/sonictool/app/chain.go Outdated Show resolved Hide resolved
@cabrador cabrador merged commit 179ec05 into develop Nov 28, 2024
2 checks passed
@cabrador cabrador deleted the c/lower-integration-tests-memory branch November 28, 2024 12:17
@LuisPH3
Copy link
Contributor

LuisPH3 commented Nov 28, 2024

🥳

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.

3 participants