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

test: refactor to allow benchmark Vote (WIP, MVP) #1028

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

altergui
Copy link
Contributor

@altergui altergui commented Jul 3, 2023

started working on #965 and opted for a refactor that will allow me reuse most of the code

@altergui altergui marked this pull request as draft July 3, 2023 21:09
@altergui altergui changed the title test: refactor to allow benchmark Vote (WIP, half-baked) test: refactor to allow benchmark Vote (WIP, MVP) Jul 4, 2023
vochain/indexer/vote.go Outdated Show resolved Hide resolved
@altergui altergui requested a review from p4u July 4, 2023 12:03
@altergui altergui self-assigned this Jul 4, 2023
@altergui altergui requested a review from mvdan July 4, 2023 12:05
qt.Assert(t, v2.VoteID.String(), qt.Equals, voter.vote.VoteID.String())
qt.Assert(t, v2.BlockHeight, qt.Equals, uint32(2))
qt.Assert(t, v2.VoterID.String(), qt.Equals, fmt.Sprintf("%x", voter.key.Address().Bytes()))
}
}

func TestAPIaccount(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's a lot of code in TestAPIaccount that could be deduplicated now, after the proposed refactoring. i'd include this in the same PR

@altergui
Copy link
Contributor Author

altergui commented Jul 4, 2023

how does it look so far? am i on the right track?

@@ -0,0 +1,26 @@
package test
Copy link
Member

Choose a reason for hiding this comment

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

@mvdan can you check the basics of this benchmark and maybe propose improvements? The idea is to have a full benchmark to test performance of state+app+indexer+api

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the logic looks okay. Two suggestions come to mind:

  • What kind of election is this - encrypted or not? We should probably benchmark the one that is most common. If both are common, then I would suggest that the benchmark do both side by side. We could similarly think about other kinds of votes, like overwrites.

  • You use a sequential benchmark via b.N, which is fine, but not very realistic - in reality, people won't vote in a big election in a big line :) For that reason, I would use a parallel benchmark via https://pkg.go.dev/testing#B.RunParallel. You can still use b.N as the total number of iterations to set up the census and anything else you need, but the looping should use https://pkg.go.dev/testing#PB.Next instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, should the benchmark advance votes while iterating? Because again, in reality, you won't have an entire election get all of its votes within a single block. Plus, the indexer does a significant amount of its work in the Commit method, so you should advance at least one block while the benchmark timer is on.

@altergui altergui force-pushed the f/apiv2_benchmarks branch from a841409 to bb0b5e0 Compare July 13, 2023 11:23
altergui added 2 commits July 13, 2023 13:26
it's failing with:
ERR vochain/app.go:287 > checkTx error="voteTx: maxCensusSize reached 0/0"
app_benchmark_test.go:131: checkTX failed: checkTx voteTx: maxCensusSize reached 0/0
@altergui altergui force-pushed the f/apiv2_benchmarks branch from 39f608a to a60c948 Compare July 13, 2023 15:11
it's failing with:
--- FAIL: BenchmarkFetchTx
    bench_test.go:157:
        error:
          got non-nil error
        got:
          e"transaction not found"
        stack:
          /home/runner/work/vocdoni-node/vocdoni-node/vochain/indexer/bench_test.go:157
            qt.Assert(b, err, qt.IsNil)
@altergui altergui force-pushed the f/apiv2_benchmarks branch from a60c948 to c17ff68 Compare July 13, 2023 15:13
@altergui altergui linked an issue Oct 17, 2023 that may be closed by this pull request
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.

vochain high level benchmark
3 participants