-
Notifications
You must be signed in to change notification settings - Fork 493
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
perf: performance test for async vote verification #5249
base: master
Are you sure you want to change the base?
perf: performance test for async vote verification #5249
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5249 +/- ##
==========================================
- Coverage 53.78% 53.70% -0.08%
==========================================
Files 450 444 -6
Lines 56201 55669 -532
==========================================
- Hits 30226 29899 -327
+ Misses 23626 23436 -190
+ Partials 2349 2334 -15 see 32 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks alright to me - few comments/suggestions
agreement/asyncVoteVerifier_test.go
Outdated
func TestAsyncVerificationVotes(t *testing.T) { | ||
partitiontest.PartitionTest(t) | ||
errProb := float32(0.5) | ||
sendReceiveVoteVerifications(false, errProb, 200, 0, t, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you make 200 and 0 named constants?
agreement/asyncVoteVerifier_test.go
Outdated
|
||
wg := sync.WaitGroup{} | ||
wg.Add(2) | ||
if b != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit/suggestion: pass T and B as a single testing.TB
interface and type assert here to B to access ResetTimer
func. There will be the same if
cond is it is now here but no if/else
at the end of the func
agreement/asyncVoteVerifier_test.go
Outdated
votes = make([]*unVoteTest, count) | ||
eqVotes = make([]*unEqVoteTest, eqCount) | ||
errsV = make(map[int]error) | ||
errsEv = make(map[int]error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: errsEqv
agreement/asyncVoteVerifier_test.go
Outdated
vi := 0 | ||
evi := 0 | ||
for c := 0; c < count+eqCount; c++ { | ||
// pick a vote if there are vots, and if either there are no eqVotes or the relative prob |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vots -> votes
agreement/asyncVoteVerifier_test.go
Outdated
|
||
nextErrType := 0 | ||
for c := 0; c < count; c++ { | ||
errType := 99 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make errType
a enum with few named values ?
type errType int
const badSig errType = 0
const noErr errType = 99
why does voteOptions
return 9 and voteEqOptions
- 10 ? what do these magic numbers mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the different invalid types produced. EqVotes have one more error.
I don't see the utility to give each number a named variable, but if you see a benefit in doing so, I would do it.
They are enumerated to loop through them and diversify the invalid types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but one of them is actually named notSelected
. Does not matter much tho but at least 99 could be named
This is a test/benchmark for async vote verification.
This is primarily to evaluated the performance gain from batching the vote verification task.