1975580 - Optimize the time marches on algorithm

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, task, P1)

Description

[Alastor Wu [:alwu]]

See bug 1974237, we have very worse performance while running time march on algorithm if a text track has huge amount of cues.

Comment 1

[Alastor Wu [:alwu]]

In bug 1974237 comment 2, running following script on this Dash player (VPN needed, not working for US)

(()=>{
  const track = $('video')[0].textTracks[0]
  const newMode = track.mode === 'showing' ? 'hidden' : 'showing'
  const before = performance.now()
  track.mode = newMode
  const elapsedTime = performance.now() - before
  const cues = track.cues?.length ?? 0;

  console.log(`Setting text track mode to ${newMode} took ${elapsedTime}ms (${cues} cues)`);
})()

The result BEFORE optimization on my debug build is

Setting text track mode to showing took 69586ms (38780 cues) (original)

The result AFTER optimization on my debug build is

Setting text track mode to hidden took 175ms (38780 cues)

That shows the improvement around 397.63x faster. I will update my patches later after finishing testing.

Comment 2

[Alastor Wu [:alwu]]

Attachment: Bug 1975580 - part1 : use nsTArray to replace TextTrackCueList.

In TimeMarchesOn, we can replace TextTrackCueList with a nsTArray, which is a
more lightweight option.

Comment 3

[Alastor Wu [:alwu]]

Attachment: Bug 1975580 - part2 : find current and other cues within a partial range.

Comment 4

[Alastor Wu [:alwu]]

Attachment: Bug 1975580 - part3 : use CueBuckets to separate active and inactive cues and avoid extra loops.

Comment 5

[Alastor Wu [:alwu]]

Attachment: Bug 1975580 - part4 : optimize the check for the pause on exit flag.

Comment 6

[Alastor Wu [:alwu]]

Attachment: Bug 1975580 - part5 : generate the missed cues concurrently while processing the other cues.

Comment 7

[Alastor Wu [:alwu]]

Attachment: Bug 1975580 - part6 : use simple double comparison to reduce overhead of creating TimeUnit.

Comment 8

[Paul Adenot (:padenot)]

(In reply to Alastor Wu [:alwu] from comment #1)

In bug 1974237 comment 2, running following script on this Dash player (VPN needed, not working for US)

(()=>{
  const track = $('video')[0].textTracks[0]
  const newMode = track.mode === 'showing' ? 'hidden' : 'showing'
  const before = performance.now()
  track.mode = newMode
  const elapsedTime = performance.now() - before
  const cues = track.cues?.length ?? 0;

  console.log(`Setting text track mode to ${newMode} took ${elapsedTime}ms (${cues} cues)`);
})()

The result BEFORE optimization on my debug build is

Setting text track mode to showing took 69586ms (38780 cues) (original)

The result AFTER optimization on my debug build is

Setting text track mode to hidden took 175ms (38780 cues)

That shows the improvement around 397.63x faster. I will update my patches later after finishing testing.

Very nice, can you compare with e.g. Chrome?

Comment 9

[Paul Adenot (:padenot)]

Also, if you have a minute, can you write in the commit message how much each patch contributes to the speed up? This is quite typical when working on performance, and helps to improve one intuition when working in optimizations in the future.

Comment 10

[Alastor Wu [:alwu]]

There’s still room for performance improvement, as Chrome runs the same script in under 5ms. I’ll continue optimizing this in Bug 1974237 and will update my commit message accordingly.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/385bb0fee1a8
https://hg.mozilla.org/integration/autoland/rev/9ea8edcb9bb8
part1 : use nsTArray to replace TextTrackCueList. r=media-playback-reviewers,padenot
https://github.com/mozilla-firefox/firefox/commit/7c675b6da89c
https://hg.mozilla.org/integration/autoland/rev/fabf5fe960ef
part2 : find current and other cues within a partial range. r=media-playback-reviewers,padenot
https://github.com/mozilla-firefox/firefox/commit/f0d24943a691
https://hg.mozilla.org/integration/autoland/rev/fadde6a5e612
part3 : use CueBuckets to separate active and inactive cues and avoid extra loops. r=media-playback-reviewers,padenot
https://github.com/mozilla-firefox/firefox/commit/24fcf09974f8
https://hg.mozilla.org/integration/autoland/rev/e619d9f4d259
part4 : optimize the check for the pause on exit flag. r=media-playback-reviewers,padenot
https://github.com/mozilla-firefox/firefox/commit/c01d6fa47a23
https://hg.mozilla.org/integration/autoland/rev/18f9a1592003
part5 : generate the missed cues concurrently while processing the other cues. r=media-playback-reviewers,padenot
https://github.com/mozilla-firefox/firefox/commit/990324ccb598
https://hg.mozilla.org/integration/autoland/rev/ac6d478c1fc9
part6 : use simple double comparison to reduce overhead of creating TimeUnit. r=media-playback-reviewers,padenot

Comment 12

[Alastor Wu [:alwu]]

We can consider uplifting this improvement to ESR, once we can confirm that those patches don’t introduce any regressions.

Comment 13

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/9ea8edcb9bb8
https://hg.mozilla.org/mozilla-central/rev/fabf5fe960ef
https://hg.mozilla.org/mozilla-central/rev/fadde6a5e612
https://hg.mozilla.org/mozilla-central/rev/e619d9f4d259
https://hg.mozilla.org/mozilla-central/rev/18f9a1592003
https://hg.mozilla.org/mozilla-central/rev/ac6d478c1fc9