Skip to content

Remove postMessage limit on audio position worklet #105541

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PizzaLovers007
Copy link

@PizzaLovers007 PizzaLovers007 commented Apr 18, 2025

This limit was added with the following comment:

// Posting messages is expensive. Let's limit the number of posts.

I ran some Chrome profiling on my test project, and on my machine (Ryzen 9 5900X, GTX 1080) the onmessage callback took 3-5 microseconds to run. Even with CPU throttling at 20x, it only took 0.2ms.

The audio JS library on no-thread Web builds posts messages with chunks of sound data, and audio.worklet.js also posts messages on every process. Limiting the position message posts doesn't seem to be necessary here.

Fixes #105397.

@PizzaLovers007 PizzaLovers007 requested a review from a team as a code owner April 18, 2025 20:12
@rburing
Copy link
Member

rburing commented Apr 19, 2025

Limiting the position message posts doesn't seem to be necessary here.

It is necessary because the PR

that introduced this limit fixed a severe game-breaking bug

I'll repeat my suggestion that position reporting should eventually be made opt-in, because in the majority of cases where you play audio you don't care about the playback position. An app that plays a single audio stream is quite a special case. In my project I had a few short sound effects that were repeated a lot, and one background music track, and in this common situation the unlimited position reporting killed performance, making the game unplayable.

The review comment on the PR linked above would be worth investigating; maybe there is an alternative solution to the problem.

@PizzaLovers007
Copy link
Author

PizzaLovers007 commented Apr 24, 2025

@rburing From my understanding of the issue, the root cause was the number of audio position worklets constantly growing, which #102163 aimed to fix by pooling the workers. The postMessage limit wasn't present in the example code from the review comment, so I'm assuming that was added later. #102715 actually implemented the suggestion from the review comment.

I downloaded the MRP from the original issue and built it on 4.4.1.stable, then manually changed the exported audio.position.worklet.js to remove the limit (setting POST_THRESHOLD_S to 0). I can't repro the bug with this setup. Could you try doing this with your project?

I will continue doing some more profiling with audio position disabled, but I suspect this is not a large factor in performance.

@PizzaLovers007
Copy link
Author

I did some more testing using the MRP (4.4.1.stable web version) on my Windows desktop (Ryzen 9 5900X, GTX 1080, 32 GB RAM).

Methodology:

  • I used Chrome's override content feature to manually edit the JS files:
    • Use worklet case: I manually set POST_THRESHOLD_S to 0 in audio.position.worklet.js. This sends a position update every process tick.
    • No worklet case: I commented out the this._source.connect(this.getPositionWorklet()); line in index.js, thus disabling the position worker.
  • After loading the game, I waited ~1 minute before clicking "Record" under the Performance tab with screenshots and memory enabled, then let the recording run for 10 seconds. I repeated 3 times for each case.
    • Example trace: chrome_2025-04-24_13-16-08
  • With each trace, I looked at time spent scripting (since this is what the PR would affect) as well as the JS memory heap.

Results from the "use worklet" case:

  1. 10128ms total, 1074ms spent scripting (10.60%). JS memory heap: 12.7 MB - 14.7 MB.
  2. 10018ms total, 1045ms spent scripting (10.43%). JS memory heap: 12.7 MB - 14.4 MB.
  3. 10065ms total, 1085ms spent scripting (10.78%). JS memory heap: 13.1 MB - 14.7 MB.

Results from the "no worklet" case:

  1. 10046ms total, 1041ms spent scripting (10.36%). JS memory heap: 12.8 MB - 14.4 MB.
  2. 10081ms total, 1070ms spent scripting (10.61%). JS memory heap: 13.6 MB - 15.2 MB.
  3. 10067ms total, 1064ms spent scripting (10.57%). JS memory heap: 14.2 MB - 15.6 MB.

Using the worklet on average increases time spent scripting from 10.51% to 10.60%, but this could be within the margin for error. In terms of optimizing for performance, this doesn't seem like a source for meaningful gains, and I would argue that having position functionality disabled would be more confusing devex.

Maybe we can get opinions from the audio team?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v4.4.1 - AudioStreamPlayer get_playback_position() updates infrequently on Web
3 participants