Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(DASH): Improve memory usage with live streams #7039

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

avelad
Copy link
Member

@avelad avelad commented Jul 15, 2024

Related to #6610

@avelad avelad added type: bug Something isn't working correctly priority: P1 Big impact or workaround impractical; resolve before feature release component: DASH The issue involves the MPEG DASH manifest format labels Jul 15, 2024
@avelad avelad added this to the v4.11 milestone Jul 15, 2024
@avelad avelad requested a review from theodab July 15, 2024 11:28
@shaka-bot
Copy link
Collaborator

Incremental code coverage: 80.00%

Copy link
Contributor

@theodab theodab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, the purpose of this PR is to remove unnecessary cases of copying the mediaTemplate string to new variables, right? Especially in cases where this copied variable might then get captured by the context of a callback?

Do you think it'd also make sense to get rid of the assignment on line 411, then? And just use the current mediaTemplate variable inside the info?
That one might not be as important since it'd just be one string captured per call to createStreamInfo, as opposed to one string captured per call to a frequently-used method like getUris, admittedly...

@avelad
Copy link
Member Author

avelad commented Jul 15, 2024

If I understand correctly, the purpose of this PR is to remove unnecessary cases of copying the mediaTemplate string to new variables, right? Especially in cases where this copied variable might then get captured by the context of a callback?

Yes that is the purpose, in my tests this reduces memory growth, but it does not eliminate it completely, but it is better than what was before.

Do you think it'd also make sense to get rid of the assignment on line 411, then? And just use the current mediaTemplate variable inside the info? That one might not be as important since it'd just be one string captured per call to createStreamInfo, as opposed to one string captured per call to a frequently-used method like getUris, admittedly...

In theory in that case it is not necessary, because we do not need to update the manifest to obtain new segments.

@avelad avelad merged commit d1435c7 into shaka-project:main Jul 15, 2024
12 of 19 checks passed
@avelad avelad deleted the dash-improve-memory-usage branch July 15, 2024 12:49
avelad added a commit that referenced this pull request Jul 15, 2024
avelad added a commit that referenced this pull request Jul 15, 2024
@cristian-atehortua
Copy link
Contributor

Yes, this was something that we were discussing on #6610. For sure, there was a memory leak on this because the created closure for getUris callback was keeping references to old string values. With this, it is ensured that the closure only keep the reference of the SegmentIndex, so when the mediaTemplate is updated, the old values can be collected.

Thanks @avelad

@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Sep 13, 2024
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Sep 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: DASH The issue involves the MPEG DASH manifest format priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants