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

Performance issues with large timeshift depths #405

Closed
torerikal opened this issue Jun 8, 2016 · 11 comments
Closed

Performance issues with large timeshift depths #405

torerikal opened this issue Jun 8, 2016 · 11 comments
Assignees
Labels
status: archived Archived and locked; will not be updated type: enhancement New feature or request
Milestone

Comments

@torerikal
Copy link

We have been testing Shaka player with live streams of different, but rather large DVR windows. We notice a significantly increased CPU load with 1-4 hour window length. With 24 hours window length, Chrome becomes unresponsive in periods.

It looks like the CPU load increases proportionally with the DVR window length. Our testing is based on some private DASH streams, and unfortunately I can't share a manifest URL.

The 24 hours DVR window length will not be subject to real use cases. Still, we are concerned about the increased CPU load for live streams with 1 to 4 hours lengths, when attempted played on medium or low end hardware, and some special use cases with other concurrent CPU load. We also look into a workaround that might exacerbate the performance problem.

Some initial CPU profiling indicates that most CPU time is spent when updating the manifest. If I were going to guess, maybe the time codes for every segment in the timeline are re-computed every time the manifest is updated? If so, do you see a workaround or strategy that could be applied here?

@tdrews tdrews added type: bug Something isn't working correctly type: enhancement New feature or request labels Jun 8, 2016
@tdrews
Copy link
Contributor

tdrews commented Jun 8, 2016

Thanks for the report.

If I were going to guess, maybe the time codes for every segment in the timeline are re-computed every time the manifest is updated?

Every segment specified in the new manifest is parsed into a SegmentReference, then after that, all those SegmentReferences are merged into the existing indexes. There are definitely some optimization opportunities, e.g., we could skip parsing segments that we know we have existing SegmentReferences for.

We'll discuss internally on what we can do here.

@joeyparrish joeyparrish removed the type: bug Something isn't working correctly label Jun 10, 2016
@raskri
Copy link

raskri commented Jun 13, 2016

Why is this not a bug? As the stream is stuttering in Chrome and not in Firefox.

@joeyparrish
Copy link
Member

If it's stuttering in Chrome and not Firefox, then one browser is executing the code faster or with less resource contention. Firefox being faster does not equal a bug in Shaka, in my opinion.

The merging code is semantically correct as far as I can tell, but we could/should make the library more efficient if we can. This is an enhancement, in my opinion.

The issue is currently tagged with "enhancement", which I believe is the best label for a potential performance improvement.

We have not investigated yet to determine what the cause is, but we'll try to get this fix into v2.0's final release. Since the code in question is completely new in v2 beta, it shouldn't affect v1.6.x, which is still the most stable release for now.

Sorry for the confusion over labels, and we'll keep you posted as we make progress.

@joeyparrish joeyparrish added this to the v2.0.0 milestone Jun 14, 2016
@peyoh
Copy link

peyoh commented Jun 20, 2016

Hi Joe,
we also observed this in v.2
Seems like this one:

#288

@TheModMaker TheModMaker self-assigned this Jun 22, 2016
shaka-bot pushed a commit that referenced this issue Jun 23, 2016
Instead of filling the URI templates when parsing the manifest,
wait until the request is made to fill it.  This reduces the time
it takes to parse the manifest.

This was tested using a stream with a 24-hour timeShiftBufferDepth.
Using a Chromebook pixel running Chrome 51.  The average manifest
parse time was about 1 second before, now it is about 200ms.

Issue #405

Change-Id: I89f36085441f6c6b7d6281b24b671dc668f23fe5
@TheModMaker
Copy link
Contributor

I just added a change to optimize the parser which increased performance in our tests. Can you try again from master and see if this fixes it?

@torerikal
Copy link
Author

Thanks, this fix seems to have a big impact.

Without the fix, I have had CPU spikes with 100 % usage for a second or two. This is eliminated when applying the fix.

@peyoh
Copy link

peyoh commented Jun 28, 2016

Thanks,
now is better.

@janouskovec
Copy link

janouskovec commented Jul 12, 2016

Would it be possible to backport this optimization to version 1.6.x? We are using v1.6.4 and we have same performance issue with large timeshift depth.

@joeyparrish
Copy link
Member

@janouskovec, we are not actively enhancing v1.6, but we will investigate and see how much trouble it would be. If it's not too complex, we will consider reimplementing in v1. Follow along at #449.

@torerikal
Copy link
Author

@janouskovec I had to create a fix for our fork of 1.6.5. Please have a look at this commit if that can help you:

vimond@adbcdb5

Not very elegant and code-style-compliant, but I hope it could be used as a basis for your own patch.

@joeyparrish
Copy link
Member

@torerikal, if you or @janouskovec would be interested in cleaning this patch up and submitting a PR to v1.6.x, I would be very happy to review it. I think overriding FailoverUri with a version that calculates this.urls at fetch() time looks pretty reasonable for v1.

Let's move discussion to #449, though, so this original issue can rest in peace. :-)

@shaka-project shaka-project locked and limited conversation to collaborators Mar 22, 2018
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Apr 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants