Improve our various round-trip timeout computations. #33587
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
woody-apple
approved these changes
May 24, 2024
|
PR #33587: Size comparison from 10f7aa9 to 7f3a359 Increases above 0.2%:
Increases (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
Decreases (2 builds for bl702, linux)
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
woody-apple
force-pushed
the
improve-timeout-computations
branch
from
7f3a359 to
c0dad30
Compare
|
PR #33587: Size comparison from 638aa28 to c0dad30 Increases above 0.2%:
Increases (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
Decreases (2 builds for bl702, linux)
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
yunhanw-google
approved these changes
May 24, 2024
bzbarsky-apple
force-pushed
the
improve-timeout-computations
branch
2 times, most recently
from
7f3a359 to
c06e84f
Compare
pullapprove
bot
requested review from
arkq,
bauerschwan,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
dhrishi,
fessehaeve,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
kkasperczyk-no,
lazarkov,
lpbeliveau-silabs and
LuDuda
pullapprove
bot
requested review from
mhazley,
mkardous-silabs,
p0fi,
plauric,
rcasallas-silabs,
saurabhst,
selissia,
sharadb-amazon,
tecimovic,
tima-q,
tobiasgraf,
wiba-nordic and
younghak-hwang
|
PR #33587: Size comparison from 21e239e to c06e84f Increases above 0.2%:
Increases (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
Decreases (2 builds for bl702, linux)
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
woody-apple
force-pushed
the
improve-timeout-computations
branch
from
c06e84f to
dc31a15
Compare
|
PR #33587: Size comparison from e2f8fc3 to dc31a15 Increases above 0.2%:
Increases (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
Decreases (2 builds for bl702, linux)
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
We had a few issues: 1) Our "round-trip timeout" only accounted for one side of the round-trip needing to do MRP retransmits. So if the sender retried a few times, the last retry finally got through, then the response had to be retried a well, the sender would time out the exchange before the response was received. The fix here is to add the MRP backoff times for both the initial message and the response. 2) ReadClient could end up timing out a subscription before the server had actually given up on receiving a StatusReport in response to its ReportData, in situations where the server ended up doing MRP retries and the last MRP retry took a few seconds to get through the network. The fix here is to just estimate how long the server will be waiting for the StatusReport and not time out the subscription until then; at that point even if the server did in fact send its report on time, it will have dropped the subscription on its end.
bzbarsky-apple
force-pushed
the
improve-timeout-computations
branch
from
dc31a15 to
dcee968
Compare
|
PR #33587: Size comparison from 6aee3fd to dcee968 Increases above 0.2%:
Increases (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
Decreases (2 builds for bl702, linux)
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
ksperling-apple
added a commit
to ksperling-apple/connectedhomeip
that referenced
this pull request
May 27, 2024
…-chip#33587)" This reverts commit 3e93ba6.
ksperling-apple
added a commit
to ksperling-apple/connectedhomeip
that referenced
this pull request
May 27, 2024
…-chip#33587)" This reverts commit 3e93ba6.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We had a few issues:
Our "round-trip timeout" only accounted for one side of the round-trip needing to do MRP retransmits. So if the sender retried a few times, the last retry finally got through, then the response had to be retried a well, the sender would time out the exchange before the response was received. The fix here is to add the MRP backoff times for both the initial message and the response.
ReadClient could end up timing out a subscription before the server had actually given up on receiving a StatusReport in response to its ReportData, in situations where the server ended up doing MRP retries and the last MRP retry took a few seconds to get through the network. The fix here is to just estimate how long the server will be waiting for the StatusReport and not time out the subscription until then; at that point even if the server did in fact send its report on time, it will have dropped the subscription on its end.