Stop doing read-throughs on MTRDevice when subscription is being established. #32990
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
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
harimau-qirex,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
kkasperczyk-no,
kpschoedel,
ksperling-apple,
lazarkov,
lpbeliveau-silabs,
LuDuda,
mhazley and
mkardous-silabs
pullapprove
bot
requested review from
selissia,
sharadb-amazon,
tcarmelveilleux,
tecimovic,
tehampson,
tima-q,
tobiasgraf,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
…blished. The key goal is to stop doing read-throughs when we expect to start getting data soon: between sending the Sigma1 for our initial subscribe and getting the first priming read data packets. Any reads dispatched during that time "because we don't have a subscription yet" are just wasted network traffic, because chances are we're about to have a subscription and get that data as part of the priming read anyway. The main fix is just to stop doing read-throughs altogether for any attributes known to not be C and if we have a subscription delegate. In that state we should be subscribed and should be getting updates for attribute values, and there's no reason to read through. The rest of the changes are related to a side-effect of the stopping of read-throughs: if we had a subscription drop and have backed off quite a bit, a read-through that succeeded could prod us to retry subscribing. We don't want to do a read just to (maybe) trigger that behavior, though. Instead, treat a skipped read-through as a direct signal to retry our subscription attempt. But not too often. This "try to resubscribe" heuristic can probably use some tweaking.
bzbarsky-apple
force-pushed
the
reduce-MTRDevice-readthrough
branch
from
812d1c8 to
30d598f
Compare
|
PR #32990: Size comparison from fd72eae to 30d598f Decreases (1 build for efr32)
Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
yunhanw-google
approved these changes
Apr 15, 2024
jtung-apple
reviewed
Apr 15, 2024
jtung-apple
approved these changes
Apr 16, 2024
|
PR #32990: Size comparison from fd72eae to 05e720a Increases above 0.2%:
Increases (17 builds for cc13x4_26x4, cc32xx, nrfconnect, nxp, qpg, stm32)
Full report (18 builds for cc13x4_26x4, cc32xx, mbed, nrfconnect, nxp, qpg, stm32)
|
bzbarsky-apple
force-pushed
the
reduce-MTRDevice-readthrough
branch
from
05e720a to
66d26a7
Compare
|
Fast-tracking platform-specific change with platform owner review. |
|
PR #32990: Size comparison from 8b86799 to 66d26a7 Decreases (1 build for efr32)
Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
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.
The key goal is to stop doing read-throughs when we expect to start getting data soon: between sending the Sigma1 for our initial subscribe and getting the first priming read data packets. Any reads dispatched during that time "because we don't have a subscription yet" are just wasted network traffic, because chances are we're about to have a subscription and get that data as part of the priming read anyway.
The main fix is just to stop doing read-throughs altogether for any attributes known to not be C and if we have a subscription delegate. In that state we should be subscribed and should be getting updates for attribute values, and there's no reason to read through.
The rest of the changes are related to a side-effect of the stopping of read-throughs: if we had a subscription drop and have backed off quite a bit, a read-through that succeeded could prod us to retry subscribing. We don't want to do a read just to (maybe) trigger that behavior, though. Instead, treat a skipped read-through as a direct signal to retry our subscription attempt. But not too often.
This "try to resubscribe" heuristic can probably use some tweaking.