Skip to content
This repository has been archived by the owner on Aug 19, 2022. It is now read-only.

Deterministic benchmark order; Keybook interface benchmarks #43

Merged
merged 3 commits into from
Nov 14, 2018

Conversation

lanzafame
Copy link
Contributor

No description provided.

License: MIT
Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>
License: MIT
Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>
License: MIT
Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>
@lanzafame
Copy link
Contributor Author

@raulk @bigs not sure if either of you have seen this, I can't request reviewers as I am not in the libp2p org.

@raulk
Copy link
Member

raulk commented Nov 7, 2018

Sorry, totally missed this as it was submitted during devcon4. On my plate for today @lanzafame.

@raulk raulk self-requested a review November 7, 2018 09:01
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

LGTM, sorry for the delay! Just minor comments that you can apply or ignore – as you wish ;-)

t.Run(name, func(t *testing.T) {
t.Run("Cacheful", func(t *testing.T) {
t.Parallel()
t.Run(name+" Cacheful", func(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep the hierarchical [datastore]/{Cacheful,Cacheless}/[testcase] format in the full test name, or is the slash a reserved char?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, didn't see this review. I will be adding some more benchmarks soon, so I will add that in then.

@@ -38,7 +39,15 @@ func BenchmarkPeerstore(b *testing.B, factory PeerstoreFactory, variant string)
go addressProducer(ctx, b, p.ch, p.n)
}

for name, bench := range peerstoreBenchmarks {
// So tests are always run in the same order.
ordernames := make([]string, 0, len(peerstoreBenchmarks))
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, although we can also change the type of peerstoreBenchmarks to be an array of struct { name string, testFunc func(...) } so the order is deterministic (I think I had it like this early on!).

Then could do away with the sorting here and in the keybook benchmarks, and apply non-lexicographical orders manually.

Not worth changing, but just wanted to point out an alternative.

@raulk raulk merged commit 0c6cf4a into libp2p:master Nov 14, 2018
@lanzafame lanzafame deleted the feat/more-benchs branch November 15, 2018 01:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants