Simplify config handling and other fixes to orchestration #170
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We skip the / separator for the first field since this is added by the t.Run(name) call.
…tuff into 159-testorchestration-cleanup
…tration/consensus=chainedhotstuff/crypto=ecdsa/byzantine=fork:1/mods=<nil>
This just reorganizes the placement of some types and methods to make the ExperimentConfig appear at the top of the file and make the order of methods more consistent. Also adds a few doc comments, and makes the receiver variable consistent across all methods on ExperimentConfig.
The old implementation of iago did a hacky solution to assign ports to docker containers that have been fixed recently. This commit bring this updated version of iago into go.mod.
meling
requested changes
Jan 20, 2025
Open
The math/rand/v2 package was added in Go 1.22 and we should use it.
Moreover, we should not use reflect.Swapper(); see code for the
very simple func()-based alternative.
This also moves the source of random values to the package level
so that we don't create a new source for every call to tree.Shuffle.
This does improve performance a bit, especially for smaller slices:
```shell
$ benchstat reflect.txt func.txt
goos: darwin
goarch: arm64
pkg: github.com/relab/hotstuff/internal/tree
cpu: Apple M2 Max
│ reflect.txt │ func.txt │
│ sec/op │ sec/op vs base │
Randomize/size=10-12 103.70n ± ∞ ¹ 66.48n ± ∞ ¹ -35.89% (p=0.008 n=5)
Randomize/size=100-12 529.8n ± ∞ ¹ 493.5n ± ∞ ¹ -6.85% (p=0.008 n=5)
Randomize/size=1000-12 4.451µ ± ∞ ¹ 4.426µ ± ∞ ¹ ~ (p=0.222 n=5)
Randomize/size=10000-12 43.16µ ± ∞ ¹ 43.17µ ± ∞ ¹ ~ (p=0.841 n=5)
geomean 1.802µ 1.582µ -12.21%
¹ need >= 6 samples for confidence interval at level 0.95
```
```shell
$ benchstat func.txt single-rnd.txt
goos: darwin
goarch: arm64
pkg: github.com/relab/hotstuff/internal/tree
cpu: Apple M2 Max
│ func.txt │ single-rnd.txt │
│ sec/op │ sec/op vs base │
Randomize/size=10-12 66.48n ± ∞ ¹ 47.59n ± ∞ ¹ -28.41% (p=0.008 n=5)
Randomize/size=100-12 493.5n ± ∞ ¹ 470.6n ± ∞ ¹ -4.64% (p=0.008 n=5)
Randomize/size=1000-12 4.426µ ± ∞ ¹ 4.411µ ± ∞ ¹ ~ (p=0.175 n=5)
Randomize/size=10000-12 43.17µ ± ∞ ¹ 43.18µ ± ∞ ¹ ~ (p=0.548 n=5)
geomean 1.582µ 1.437µ -9.18%
¹ need >= 6 samples for confidence interval at level 0.95
```
```shell
$ benchstat reflect.txt single-rnd.txt
goos: darwin
goarch: arm64
pkg: github.com/relab/hotstuff/internal/tree
cpu: Apple M2 Max
│ reflect.txt │ single-rnd.txt │
│ sec/op │ sec/op vs base │
Randomize/size=10-12 103.70n ± ∞ ¹ 47.59n ± ∞ ¹ -54.11% (p=0.008 n=5)
Randomize/size=100-12 529.8n ± ∞ ¹ 470.6n ± ∞ ¹ -11.17% (p=0.008 n=5)
Randomize/size=1000-12 4.451µ ± ∞ ¹ 4.411µ ± ∞ ¹ -0.90% (p=0.032 n=5)
Randomize/size=10000-12 43.16µ ± ∞ ¹ 43.18µ ± ∞ ¹ ~ (p=0.841 n=5)
geomean 1.802µ 1.437µ -20.27%
¹ need >= 6 samples for confidence interval at level 0.95
```
Just moved the output check first: - allows faster error out, if the output path is bad - avoids cfg.Output assignments
This was referenced Jan 22, 2025
… intended to be used in tests. This is what caused the test fails.
This replaces the string-based byzantine specification of things like silent:1 and slow:1. This is more specific and cannot result in parse errors.
meling
approved these changes
Jan 23, 2025
AlanRostem
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #168
Previously, the
--hostscli flag was omitted when integrating Cue. Now I have introduced--client-hostsand--replica-hoststo the cli so that things can work like before and be compatible with Cue if the user chooses to.Both Cue and Viper (.toml) are compatible with the framework.
The code has also been adapted to support the following:
ExperimentConfigstruct that holds all possible parameters.toml, and.cueconfigs can work in tandemThis PR also solves #150, #159, and #174