[Darwin] MTRDevice should throttle writes to the attribute storage #33535
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
|
PR #33535: Size comparison from 70dc5a8 to 8c0085a Decreases (1 build for efr32)
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
|
PR #33535: Size comparison from eb515e1 to 2725771 Decreases (1 build for efr32)
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
jtung-apple
force-pushed
the
jtung/darwin-mtrdevice-attribute-storage-write-optimization
branch
from
2725771 to
1e461a5
Compare
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
fessehaeve,
harimau-qirex,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo and
kiel-apple
pullapprove
bot
requested review from
tcarmelveilleux,
tecimovic,
tehampson,
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic and
younghak-hwang
jtung-apple
force-pushed
the
jtung/darwin-mtrdevice-attribute-storage-write-optimization
branch
from
1e461a5 to
4f7333e
Compare
|
PR #33535: Size comparison from c3ef110 to 4f7333e Decreases (1 build for efr32)
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
|
PR #33535: Size comparison from c3ef110 to 285b2b1 Increases above 0.2%:
Increases (13 builds for cyw30739)
Decreases (9 builds for cyw30739, efr32)
Full report (52 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32)
|
woody-apple
approved these changes
May 23, 2024
|
PR #33535: Size comparison from c3ef110 to 59bf484 Increases above 0.2%:
Increases (13 builds for cyw30739)
Decreases (9 builds for cyw30739, efr32)
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
|
PR #33535: Size comparison from c3ef110 to 9ad4922 Increases above 0.2%:
Increases (14 builds for cyw30739, efr32)
Decreases (9 builds for cyw30739, efr32)
Full report (52 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32)
|
|
PR #33535: Size comparison from c3ef110 to 9a4f3e4 Increases above 0.2%:
Increases (14 builds for cyw30739, efr32)
Decreases (10 builds for cyw30739, efr32, linux)
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
|
PR #33535: Size comparison from c3ef110 to d9633ee Increases above 0.2%:
Increases (14 builds for cyw30739, efr32)
Decreases (10 builds for cyw30739, efr32, linux)
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
…n the machine is slow
… fails the test when the machine is slow
jtung-apple
force-pushed
the
jtung/darwin-mtrdevice-attribute-storage-write-optimization
branch
from
29ba757 to
d8f14fa
Compare
|
PR #33535: Size comparison from 57f29a8 to d8f14fa Decreases (1 build for efr32)
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
| @@ -423,11 +441,27 @@ - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceControlle | |||
| } | |||
| _clusterDataToPersist = nil; | |||
| _persistedClusters = [NSMutableSet set]; | |||
|
|
|||
| // If there is a data store, make sure we have an observer to | |||
| _deviceReportingExcessivelyStartTime = nil; | ||
| _reportToPersistenceDelayCurrentMultiplier = 1; | ||
|
|
||
| if (_persistedClusters) { |
There was a problem hiding this comment.
This is an odd check. Why is this not just unconditional (and then no-op if there is nothing to persist right now)? Needs comments, at least.
| NS_ASSUME_NONNULL_BEGIN | ||
|
|
||
| /** | ||
| * Class that configures how MTRDevice objects persist its attributes to storage, so as to not |
There was a problem hiding this comment.
"their" not "its", since it's "objects"?
| * When the running average time between reports dips below timeBetweenReportsTooShortMinThreshold | ||
| * for the first time, the time will be noted. If the device remains in this state for longer than | ||
| * deviceReportingExcessivelyIntervalThreshold, persistence will stop until the average time between | ||
| * reports go back above timeBetweenReportsTooShortMinThreshold. |
There was a problem hiding this comment.
"goes back".
In terms of the implementation, if the device is reporting excessively for a bit, and then stops reporting entirely for a long time, shouldn't we persist at some point? Right now, I don't think we do....
Seems to me like we should compute when the average time would go back above if we got no reports until then and schedule something to persist at that point or something.
| * be used to replace all of them: | ||
| * | ||
| * reportToPersistenceDelayTimeDefault (15) | ||
| * reportToPersistenceDelayTimeMaxDefault (20 * kReportToPersistenceDelayTimeDefault) |
There was a problem hiding this comment.
kReportToPersistenceDelayTimeDefault is not a concept in this API... Should just say 20 * 15, I think.
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.
This change makes MTRDevice throttle writing to the attribute storage. It has three parts:
Currently working on unit tests, and fixing the unit tests that this broke.