Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

Add benchmark for BlockHashOperation #203

Merged
merged 4 commits into from
Oct 29, 2018

Conversation

ajsutton
Copy link
Contributor

PR description

Adds a micro-benchmark for the BlockHashOperation and a reusable OperationBenchmarkHelper to make it easier to write benchmarks for other operations.

public class OperationBenchmarkHelper {

private final Path storageDirectory;
private final RocksDbKeyValueStorage keyValueStorage;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be RocksDbKeyValueStorage? Can we use the abstract parent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need access to the close method which is only on RocksDbKeyValueStorage. I think I might follow up with another PR to add close to the KeyValueStorage interface which would improve this kind of thing in a few places, but outside this scope of this change.


public static OperationBenchmarkHelper create() throws IOException {
final Path storageDirectory = Files.createTempDirectory("benchmark");
final RocksDbKeyValueStorage keyValueStorage = RocksDbKeyValueStorage.create(storageDirectory);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use Provider and pass it into create?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My aim here is to make pull as much of the complexity out of the benchmark classes where it would have to be repeated into this helper class where it can be shared. This also isn't the type of benchmark where you'd test out multiple back-ends because the usage of storage is typically very light (even SSTORE wouldn't affect the key value storage in this scope).

Copy link
Contributor

@shemnon shemnon left a comment

Choose a reason for hiding this comment

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

Approved with reservations. We need to make sure we aren't tied too deeply to a specific Key Value Storage instance. Adrian said that is outside the scope of this PR but we need to address it before too long.

@ajsutton ajsutton merged commit e56125c into PegaSysEng:master Oct 29, 2018
@ajsutton ajsutton deleted the blockhashop-benchmark branch October 29, 2018 19:59
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.

2 participants