-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
DASH processManifest performance regression in 3.x #4062
Comments
I did some profiling and code reading and I strongly suspect a refactoring which happened between 2.5 and 3:
Example in // Shaka 2.5.x
for (let i=0; i < timePoints.length; ++i) {
// Shaka 3.x
const enumerate = (it) => shaka.util.Iterables.enumerate(it);
for (const {item: timePoint, next} of enumerate(timePoints)) { This looks like an absolutely catastrophic refactoring idea. |
@joeyparrish / @theodab , can you check this? If so, I think it should be changed to improve performance. |
I have created #4064 to fix these issues. |
Amazing @avelad - I was precisely doing these changes locally to see if it was fixing the issue :D |
@philippe-elsass-deltatre Have you seen performance improvements with the fix? |
@avelad when profiling with Chrome browser there is a 10x improvement at least. |
However I'm seeing a worrying climbing of the memory |
Have you seen any memory leak? |
There seem to be a massive memory leak: even after unloading Shaka I see memory climbing and if I pause JS I end up in a |
And is it caused by this change or is it reproduced in the master branch? |
I don't know that's odd - the player seem to be leaking if I build from source tag v3.3.2. Maybe I'm doing it wrong. |
Is there something faster than |
No I'm good, building from source is fine for |
Ok finally your branch doesn't leak - I did have something wrong. Performance-wise with release JS the branch is about 40% faster - I'll try that on device now. |
So... I under-estimated the penalty of the debug JS and my alarming "10s" on LG was a lot due to being a debug build (I couldn't find where to add timings otherwise). Still the branch brings is a major improvement on LG device. |
Unfortunately Shaka 2.5.9 is still MUCH faster. Our LGs stutter with a live stream with 4h DVR with Shaka 3 (even optimised) while Shaka 2 handles it without a sweat. |
The good thing is that the fix is good, although it does not improve everything that it should, but it would be necessary to see what other parts can be optimized. |
I think that PR is an improvement but this issue should keep open for more investigation |
I have done some analysis and have concluded that ES6 generators, even when not transpiled to ES5, are monstrously slow. We use them in two methods of Here's a comparison of three ways to iterate through a large array of 100k items:
The results are shocking. The first two methods are similar (159.8 runs / second vs 176.2 runs / second). So the compiler does not seem to add much in the way of overhead (around 10%). But the third method, which is only two lines difference from the second, runs at 3141.2 runs / second, or 17.8x faster. So we need to get rid of all generator usage and ban it at the compiler level. To reproduce these results, open https://jsbench.me/jnl16wbdor/1 and click "Run" at the top. Keep the tab in the foreground, or else the results will be thrown off by throttling in the browser. |
@philippe-elsass-deltatre, please try this build which removes all generators (and makes no other changes):
If that is a meaningful improvement in performance for you, we can pursue something like this as a solution. (Though this is just a drop-in, API compatible replacement of generators with returned Arrays. We may want to evaluate individual cases and make more changes like @avelad's.) |
Nice - I've never been a big fan of generators but now I have an excuse :D One thing I notice in Shaka 3 VS Shaka 2 is MUCH larger times spent in GC. An |
Looks like the release bot closed this issue when it shouldn't have. |
Hi, just looking at this issue because we are facing a big rebuffering problem on webOS devices 3.x and 4.4. I saw the function that is wasting more time to execute is I was thinking that a good approach to solve the issue is to make this processing on demand, only when the segment is about to be used. I know this implies a big refactor on the way SegmentIndex works but maybe you can take this as a reference to improve it. What do you think? |
Thanks for the hint! Yes, I think you're right that this would be a big refactor to SegmentIndex. Perhaps we can find a new way to abstract SegmentReference to make this work. A parse-on-demand structure would require us to keep the XML elements in memory, but perhaps this would still be a win. |
I think the problem has been mitigated with the latest versions. can you check it? |
Hi @avelad, I didn't look to perform better in the latest versions. We found that this has a special impact when a live manifest has more than one audio track. It looks like Shaka parses each AdaptationSet even if that audio track is not selected. Maybe a "quick" win could be to delay parsing of the audio tracks until they're selected. What do you think @joeyparrish? |
We already added support for delaying the parsing of HLS media playlists, so some of the work for variants not fully loading until chosen is already done. |
Well, based on the workaround we applied I think delaying could be a practical solution. To workaround on the issue we preprocess the manifest and we just keep a single audio track, deleting completely the AdaptationSets that don't match the user's language preference. This is a feature loss but we had to do it because if not, our streams will not be playable. I think we can avoid the feature loss if Shaka optimize the way it process the manifest. Just to clarify, this happens with our live streams that has the representation with SegmentTimeline and only on WebOS v3 and v4 since this platforms looks to be very limited in resources. |
Well, I don't think it will have memory savings as great as preprocessing the manifest by removing entire tracks. We will need to keep the stuff we haven't parsed in memory, after all, so it can be parsed later. But I think it's probably worth a try. I have other higher-priority issues I need to get to first, though, so I won't be able to work on this right away. |
I think the problem has been mitigated in the main branch (see f1c5a1c), can you check it? Thanks! |
Closing due to inactivity. If this is still an issue for you or if you have further questions, the OP can ask shaka-bot to reopen it by including |
Have you read the FAQ and checked for duplicate open issues?
Yes
What version of Shaka Player are you using?
2.5.x, 3.x
Can you reproduce the issue with our latest release version?
Yes
Can you reproduce the issue with the latest code from
main
?Didn't try
Are you using the demo app or your own custom app?
Custom
If custom app, can you reproduce the issue using our demo app?
N/A
What browser and OS are you using?
webOS, Tizen
For embedded devices (smart TVs, etc.), what model and firmware version are you using?
webOS 5.00.35
Tizen firmware 2200.9
What are the manifest and license server URIs?
What configuration are you using? What is the output of
player.getConfiguration()
?Default configuration
What did you do?
Play a live stream with large DVR (several hours) using different versions of Shaka Player on webOS and Tizen TV (low performance devices)
What did you expect to happen?
Playback should be smooth
What actually happened?
Using Shaka Player 2.5.x, Shaka is able to load and process the growing live manifest every few seconds.
Using Shaka Player 3.x, the
processManifest
step takes several seconds.With a 4+h DVR, every time the manifest is reloaded:
There is visibly a considerable performance degradation of the manifest parsing logic, making it barely tolerable on Tizen (UI is frozen during the parsing), and completely unusable on webOS.
The text was updated successfully, but these errors were encountered: