performance: dynamic lambda #5654
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5654 +/- ##
==========================================
+ Coverage 55.51% 55.55% +0.04%
==========================================
Files 473 474 +1
Lines 66316 66416 +100
==========================================
+ Hits 36814 36899 +85
- Misses 27005 27013 +8
- Partials 2497 2504 +7
... and 12 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@yossigi |
| // the linear time order statistics algorithm. | ||
| sortedArrivals := make([]time.Duration, len(history.history)) | ||
| copy(sortedArrivals[:], history.history[:]) | ||
| sort.Slice(sortedArrivals, func(i, j int) bool { return sortedArrivals[i] < sortedArrivals[j] }) |
There was a problem hiding this comment.
I'm told slices.Sort() is a bit faster because the bounds checks in the generated code end up better optimized. (Also slightly easier to write/read the code.)
| // Round and Period are the round and period for which to query the | ||
| // lowest-credential vote, value or payload. This type of event is only | ||
| // sent for reading the lowest period 0 credential, but the Period is here | ||
| // anyway to route to the appropriate proposalMachinePeriod. | ||
| Round round | ||
| Period period |
There was a problem hiding this comment.
How does this Period=0 thing working? I don't see how the calling code can work generically across event types, so it can't just pluck ev.Period out of an arbitrary event of any type.
| re := readLowestEvent{T: readLowestVote, Round: p.Round, Period: p.Period} | ||
| re = r.dispatch(*p, re, proposalMachineRound, p.Round, p.Period, 0).(readLowestEvent) |
There was a problem hiding this comment.
I guess my earlier comment is saying that we might as well not have the ev.Period field in the re, and just call r.dispatch with a 0 on line 290.
| re := e.(readLowestEvent) | ||
| return r.dispatch(p, re, proposalMachinePeriod, re.Round, re.Period, 0).(readLowestEvent) |
There was a problem hiding this comment.
Again, we would call dispatch with a hard-coded 0.
| history.full = false | ||
| } | ||
|
|
||
| // isFull checks if the circular buffer has been fully populated at least once. |
There was a problem hiding this comment.
I suppose this is a bit silly, but if you want the 37th best of 40 and you're trying to avoid answering too low, you could start answering once you have 37 samples, and return the worst. We could call this isReady, and you have to initialize the struct with the statistic you care about (37), rather than just the total size. And you'd only support .GetStatistic() rather that all the caller to say which one it wants.
Summary
This PR changes the filter timeout on period 0 to be calculated dynamically, based on the history of credential arrival times. This allows reducing the time a node waits during the filter step on this period. If the filter timeout ends up being too short for too many nodes, preventing consensus, the protocol continues to period 1 where we fall back on the original filter timeout value.
Test Plan
Added new tests for player to make sure it reads the time samples correctly.
Added new tests for service to make sure it calculates the time samples correctly.
Added tests for new closed components, such as the credentialArrivalHistory.