chore: Improve Sonos Memory Usage #2306
Draft
Conversation
This file contains hidden or 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
We have historically ignored non-primary devices in bonded sets during discovery/onboarding, going back to the pre-Edge days. Bonded sets are things like stereo pairs or home theater setups. These devices cannot be controlled via the WSS LAN API at all, and they aren't treated as being part of a Group in the Sonos group model; the intent is for API consumers to treat the entire bonded set as a single "player". Only one of the devices in the set exposes an API endpoint, which differs from a Group where all the players expose an endpoint. While groups service most commands via calling the API endpoint on the Group Coordinator, certain commands like volume can be sent to non-coordinator players. This is not the case with bonded sets; the entire set always acts atomically, and only the primary device in the set can service commands. Hence, we treat non-primary devices as not controllable in a bonded set, and we don't onboard them. However, we didn't really have very robust handling of devices that become part of a bonded set at runtime after onboarding. This led to unnecessary CPU and RAM usage by creating a few tight loops due to unexpected failure/edge cases when making certain calls. Because we would fail to discover this device under normal circumstances, the preferred way to handle this transition is to mark the device as being offline. We discussed emitting a delete for these devices, but it seems that it wouldn't be too uncommon for some users to create and destroy bonded sets ephemerally the way one might do with a Group. For this reason we decided we would avoid deleting the records. If a non-primary in a bonded set is removed from the bonded set, it will be marked as online again.
@NoahCornell did some analysis of driver heap usage, and Sonos was disproportionately above the mean for the heap-space-per-device line fit on the dataset. This PR improves memory usage by limiting the information we keep resident from API responses to the fields that we actually use in regular operation. Additional background: One source of heap usage was unbounded memory growth due to task spawning on a certain error pathway, which he already fixed in a different PR. Another source was the fact that we were cacheing the entire API response for the Player and Group information for the LAN's Sonos topology. These API payloads are pretty large, and have actually gotten larger over time. This has been the case for this driver for a very long time; the decision to store the whole response object was made so that we would have the information available if we needed it in the future for bug fixes or enhancements. It turns out that the information we're utilizing hasn't really changed much over the last few years, so I'm feeling quite comfortable about excising the majority of the payload information at this point. We see pretty signifcant memory savings with these changes, and the savings should scale appreciably with device count, which is a big win.
|
Invitation URL: |
Test Results 68 files 447 suites 0s ⏱️ Results for commit 5a92f43. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 5a92f43 |
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.
Type of Change
Checklist
Depends On
Description of Change
@NoahCornell did some analysis of driver heap usage, and Sonos was
disproportionately above the mean for the heap-space-per-device line fit
on the dataset.
This PR improves memory usage by limiting the information we keep resident
from API responses to the fields that we actually use in regular operation.
Additional background:
One source of heap usage was unbounded memory growth due to task spawning
on a certain error pathway, which he already fixed in a different PR.
Another source was the fact that we were cacheing the entire API
response for the Player and Group information for the LAN's Sonos topology.
These API payloads are pretty large, and have actually gotten larger over time.
This has been the case for this driver for a very long time; the
decision to store the whole response object was made so that we would
have the information available if we needed it in the future for bug
fixes or enhancements.
It turns out that the information we're utilizing hasn't really changed
much over the last few years, so I'm feeling quite comfortable about
excising the majority of the payload information at this point. We see
pretty signifcant memory savings with these changes, and the savings
should scale appreciably with device count, which is a big win.
Summary of Completed Tests
Tested on my personal setup by overriding the driver files directly. Will need to be regression tested by internal QA once this lands on Alpha; the OAuth stuff precludes this from being tested using the PR channel invite.