Shard validator info handler map interface #3907
Conversation
mariusmihaic
changed the title
| // CreateShardValidatorsMap creates an instance of shardValidatorsInfoMap which manages a shard validator | ||
| // info map internally. | ||
| func CreateShardValidatorsMap(input map[uint32][]*ValidatorInfo) *shardValidatorsInfoMap { | ||
| ret := &shardValidatorsInfoMap{valInfoMap: make(map[uint32][]ValidatorInfoHandler, len(input))} |
There was a problem hiding this comment.
also here, we do not need the call to len(input)
There was a problem hiding this comment.
I think here it is ok to call len to allocate memory for the map, since they will have the same size, right?
|
|
||
| // GetShardValidatorsInfoMap returns a <shard, ValidatorInfoHandler> map copy of internally stored data | ||
| func (vi *shardValidatorsInfoMap) GetShardValidatorsInfoMap() map[uint32][]ValidatorInfoHandler { | ||
| ret := make(map[uint32][]ValidatorInfoHandler, len(vi.valInfoMap)) |
There was a problem hiding this comment.
Same comment as above. This link also states something about it:
https://stackoverflow.com/questions/53388332/memory-efficient-implementation-of-go-map
Go maps are optimized for integer keys. Optimize the map allocation by giving the exact map size as a hint.
| // Add adds a ValidatorInfoHandler in its corresponding shardID | ||
| func (vi *shardValidatorsInfoMap) Add(validator ValidatorInfoHandler) error { | ||
| if check.IfNil(validator) { | ||
| return ErrNilValidatorInfo |
| vi.mutex.Lock() | ||
| defer vi.mutex.Unlock() | ||
|
|
||
| for idx, validator := range vi.valInfoMap[shardID] { |
There was a problem hiding this comment.
slight optimization:
validatorSlice := vi.valInfoMap[shardID]
for idx, validator := range validatorSlice {
....There was a problem hiding this comment.
Actually, it seems like this is very slightly less efficient. I've run a benchmark on a map with 1,000,000 entries in which
Iterate1iterates usingfor range vi.valInfoMap[shardID]Iterate2iterates using:
vSlice := vi.valInfoMap[shardID]
for range vSlice {
}Results:
BenchmarkShardValidatorsInfoMap_IterateMap1/iterate_1
BenchmarkShardValidatorsInfoMap_IterateMap1/iterate_1-8 1000000000 0.0002455 ns/op
BenchmarkShardValidatorsInfoMap_IterateMap1/iterate_2
BenchmarkShardValidatorsInfoMap_IterateMap1/iterate_2-8 1000000000 0.0002704 ns/op
| defer vi.mutex.RUnlock() | ||
|
|
||
| for shardID, validatorsInShard := range vi.valInfoMap { | ||
| validatorsCopy := make([]ValidatorInfoHandler, len(validatorsInShard)) |
There was a problem hiding this comment.
👍 by always using copies of slices you prevented the wrongly usage of this component, by directly making a slice element to nil that would have altered the inner map's components
There was a problem hiding this comment.
Right, this was the intention !
bogdan-rosianu
requested review from
bogdan-rosianu
and removed request for
bogdan-rosianu
ShardValidatorsInfoMapHandlerinterface which shall be used to manage operations inside a<shardID,[]ValidatorInfoHandler>map in a concurrent-safe way. This is used to replace operations with pointers on a<shardID, []*ValidatorInfo>map. Integration PRs with this interface will follow. For now, basic functionalities have been provided, this interface being subject to change in future PRsValidatorInfoHandler(similar withHeaderHandler- getters + setters) interface to be used instead of*ValidatorInfo