Max-cover: minor optimizations #8311
Conversation
| return atts, nil | ||
| } | ||
| return aggregated.merge(unaggregated), err | ||
| } |
There was a problem hiding this comment.
NewMaxCover() doesn't return error now, since it is used in a loop over the same set of attestations, and it is more optimal to validate that set of attestations once, before loop starts (which we indeed do, see line 25 above).
| } | ||
| } | ||
|
|
||
| func NewMaxCover(atts []*ethpb.Attestation) *aggregation.MaxCoverProblem { |
There was a problem hiding this comment.
Just removing validation in a new method.
| i++ | ||
| } | ||
| return merged | ||
| return append(al, al1...) |
There was a problem hiding this comment.
I am not sure what I was thinking about when I implemented manual merging of two slices, when append() has us covered (if future the whole attList.merge() method might be removed, and we will just be inlining append(), but for now wanted to keep change constrained).
| @@ -171,7 +144,7 @@ func (al attList) filterContained() attList { | |||
| sort.Slice(al, func(i, j int) bool { | |||
| return al[i].AggregationBits.Count() > al[j].AggregationBits.Count() | |||
| }) | |||
| filtered := make([]*ethpb.Attestation, 0, len(al)) | |||
| filtered := al[:0] | |||
There was a problem hiding this comment.
Just relying on fact that al[:0] will be sharing the same underlying array, so no need to allocate anything - we are ok wit in-place amendment (see https://github.com/golang/go/wiki/SliceTricks#filtering-without-allocating)
What type of PR is this?
What does this PR do? Why is it needed?
Bitlist64based implementation of max-cover will be pushed).prysm/shared/aggregation/attestations/maxcover.go
Lines 25 to 30 in 9cc1438
append()Which issues(s) does this PR fix?
Part of #7871, extracted from #7938
Other notes for review
Original benchmark:
After applying this PR's patch:
