Speculative execution fixes #979
Conversation
spec says: ``` ratio of peer responses after which speculation is used. Set to 1 to disable. speculation-threshold = 1 ``` However, code would only trigger after *more* than the threshold is met. example: 4 shardgroups. threshold = 0.75 imagine after 3 shardgroups have returned, you would expect the speculation to kick in. However, the logic would flow like this: percentReceived = 1 - (1/4) = 3/4 = 0.75 if percentReceived > speculationThreshold then speculate if 0.75 > 0.75 -> no speculation!. It would wait until another shard returns (in this case, until all shard groups return) also if threshold=1, bypass speculation logic more explicitly
| // Check if it's time to speculate! | ||
| percentReceived := 1 - (float64(len(pendingResponses)) / float64(len(peerGroups))) | ||
| if percentReceived > speculationThreshold { | ||
| percentReceived := float64(len(receivedResponses)) / float64(len(peerGroups)) |
There was a problem hiding this comment.
Is pendingResponses still worth anything? It seems superfluous. The current uses are:
- To drive the main loop (
for len(pendingResponses) > 0) which could befor len(receivedResponses) < len(peerGroups) - To decide upon who to speculate which could loop on
peerGroupsand continue if inrecievedResponses.
I'm not sure it's worth the extra allocation.
| specCheckTicker := time.NewTicker(5 * time.Millisecond) | ||
| var ticker *time.Ticker | ||
| var tickChan <-chan time.Time | ||
| if speculationThreshold != 1 { |
There was a problem hiding this comment.
that's nice. so the ticker doesn't even get instantiated if speculationThreshold == 1
| // Check if it's time to speculate! | ||
| percentReceived := 1 - (float64(len(pendingResponses)) / float64(len(peerGroups))) | ||
| if percentReceived > speculationThreshold { | ||
| percentReceived := float64(len(receivedResponses)) / float64(len(peerGroups)) |
There was a problem hiding this comment.
that's changing the logic, but it's actually making it correct, so that's good.
There was a problem hiding this comment.
What's the change in logic?
There was a problem hiding this comment.
this particular line should not change logic. but the line below it does.
i explained in depth in the commit message
(i generally try to make commit messages explain everything)
There was a problem hiding this comment.
Yes, that was my analysis as well 👍
No description provided.