Conversation
lite2/client_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func BenchmarkBisection(b *testing.B) { |
There was a problem hiding this comment.
979-1005 lines are duplicate of lite2/client_test.go:1007-1034 (from dupl)
lite2/client_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func BenchmarkRecursiveBisection(b *testing.B) { |
There was a problem hiding this comment.
1007-1034 lines are duplicate of lite2/client_test.go:979-1005 (from dupl)
lite2/client_test.go
Outdated
| ) | ||
|
|
||
| func BenchmarkSequence(b *testing.B) { | ||
| b.N = 10 |
There was a problem hiding this comment.
SA3001: should not assign to b.N (from staticcheck)
lite2/client_test.go
Outdated
| } | ||
|
|
||
| func BenchmarkBisection(b *testing.B) { | ||
| b.N = 1000 |
There was a problem hiding this comment.
SA3001: should not assign to b.N (from staticcheck)
lite2/client_test.go
Outdated
| } | ||
|
|
||
| func BenchmarkRecursiveBisection(b *testing.B) { | ||
| b.N = 1000 |
There was a problem hiding this comment.
SA3001: should not assign to b.N (from staticcheck)
|
Here are some preliminary results of the benchmarking. I am still not happy with the amount of variance. So I want to look into it some more. It seems that the two bisection methods are relatively similar in performance when compared with the sequence method |
Codecov Report
@@ Coverage Diff @@
## master #4514 +/- ##
=========================================
+ Coverage 65.33% 65.4% +0.07%
=========================================
Files 229 229
Lines 20287 20323 +36
=========================================
+ Hits 13254 13292 +38
+ Misses 5979 5975 -4
- Partials 1054 1056 +2
|
lite2/client_test.go
Outdated
| } | ||
|
|
||
| // | ||
| // ################################# BENCHMARKING ###################################### |
There was a problem hiding this comment.
let's move into a separate file (client_benchmark_test.go)
lite2/client_test.go
Outdated
|
|
||
| func BenchmarkSequence(b *testing.B) { | ||
| c := defaultClient() | ||
| trustedHeader, _, err := c.fetchHeaderAndValsAtHeight(1) |
There was a problem hiding this comment.
can't we do this with public API?
lite2/client_test.go
Outdated
| require.NoError(b, err) | ||
| b.ResetTimer() | ||
| for n := 0; n < b.N; n++ { | ||
| _ = c.sequence(trustedHeader, untrustedHeader, untrustedVals, bTime.Add(1000*time.Minute)) |
There was a problem hiding this comment.
can't we do this with public API?
There was a problem hiding this comment.
I guess I just wanted to test just the pure verification method not the entire thing - but I could change it so it uses VerifyHeaderAtHeight() instead
lite2/client_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func BenchmarkClientInitialization(b *testing.B) { |
There was a problem hiding this comment.
don't think we need this per se
There was a problem hiding this comment.
okay I will remove it
|
|
||
| if !bytes.Equal(untrustedHeader.ValidatorsHash, untrustedVals.Hash()) { | ||
| return errors.Errorf("expected new header validators (%X) to match those that were supplied (%X)", | ||
| return errors.Errorf("expected new header validators (%X) to match those that were supplied (%X) at height %d", |
There was a problem hiding this comment.
it's the responsibility of the caller to add this info since a) it can be obtained outside this function b) function itself does not change the value of height param
There was a problem hiding this comment.
What do you mean by this - do you thing adding at heigh %d is unnecessary
There was a problem hiding this comment.
do you thing adding at heigh %d is unnecessary
yes
cmwaters
left a comment
There was a problem hiding this comment.
Will make changes
Closes: #4504 and #4392
Description
Created 4 benchmarking tests testing:
@melekes let me know what you think
For contributor use:
docs/) and code commentsFiles changedin the Github PR explorer