-
Notifications
You must be signed in to change notification settings - Fork 22
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
Improve performance for test generation and execution #359
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
types/testingutils/keymanager.go
Outdated
) | ||
|
||
func getHash(data [][]byte) string { | ||
hasher := sha256.New() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can consider using MuHash or something else which is faster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fnv is easy fast and simple for tasks like this. built in the go stdlib
types/testingutils/keymanager.go
Outdated
mu.Lock() | ||
defer mu.Unlock() | ||
|
||
hash := getHash(slashableDataRoots) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better approach would be getHash
before locking, so we keep it locked as short as possible
types/testingutils/keymanager.go
Outdated
var ( | ||
instancesMap = make(map[string]*testingKeyManager) | ||
mu sync.Mutex | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally I would say global state is a bad idea, but since this is a test generation code and you are protecting it properly, its seems ok to me.
Overview
The purpose of this PR is to optimize the performance of test generation and execution with the purpose of improving work quality on future changes.
The
ssv
module is the one that takes the most time. After running a CPU profiling test, we noticed that theNewTestingKeyManager
function consumes 80% of the total time, which is explained by the fact that it's called multiple times and takes 26 milliseconds to run (according to benchmark tests).Nonetheless, the
NewTestingKeyManager
function returns atestingKeyManager
object that is only used to validate and sign data but does not change state throughout a test execution. Thus, we propose adding a singleton pattern for this object.Metrics
To illustrate the number of times the
NewTestingKeyManager
function is called, take a look at the following tableNote: the number after the dash (/) is the number of different instances created, which depends on a
slashableDataRoots
parameter.Performance comparison
With the singleton pattern, if the appropriate object already has been created, the function takes 5 microseconds, rather than 26 milliseconds.
All time values shown below are in seconds (or in minutes:seconds if it has ":" ).
For test generation, we got the following performance results.
For test execution, we got the following performance results.