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

perf: performance test for async vote verification #5249

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

algonautshant
Copy link
Contributor

This is a test/benchmark for async vote verification.
This is primarily to evaluated the performance gain from batching the vote verification task.

@codecov
Copy link

codecov bot commented Apr 2, 2023

Codecov Report

Merging #5249 (93687be) into master (f4f5ec6) will decrease coverage by 0.08%.
The diff coverage is n/a.

❗ Current head 93687be differs from pull request most recent head f9145a8. Consider uploading reports for the commit f9145a8 to get more accurate results

@@            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

Copy link
Contributor

@algorandskiy algorandskiy left a 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 Show resolved Hide resolved
func TestAsyncVerificationVotes(t *testing.T) {
partitiontest.PartitionTest(t)
errProb := float32(0.5)
sendReceiveVoteVerifications(false, errProb, 200, 0, t, nil)
Copy link
Contributor

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?


wg := sync.WaitGroup{}
wg.Add(2)
if b != nil {
Copy link
Contributor

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

votes = make([]*unVoteTest, count)
eqVotes = make([]*unEqVoteTest, eqCount)
errsV = make(map[int]error)
errsEv = make(map[int]error)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: errsEqv

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

Choose a reason for hiding this comment

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

vots -> votes


nextErrType := 0
for c := 0; c < count; c++ {
errType := 99
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@algorandskiy algorandskiy Apr 18, 2023

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

algorandskiy
algorandskiy previously approved these changes Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants