Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Oct 26, 2025

Also:

  • Removing duplication in precondition check.
  • Use dbg() for debug messages

The use of the HandleAllocator does add about 70 bytes to the JS
payload, but that will be amortized over the different places where
HandleAllocator is also uses.

@sbc100 sbc100 requested a review from kripken October 26, 2025 00:23
@sbc100 sbc100 force-pushed the libwebaudio branch 4 times, most recently from 2be5458 to a7f761b Compare October 26, 2025 01:15
Also:

- Removing duplication in precondition check.
- Use `dbg()` for debug messages

The use of the HandleAllocator does add about 70 bytes to the JS
payload, but that will be amortized over the different places where
HandleAllocator is also uses.
@juj
Copy link
Collaborator

juj commented Oct 26, 2025

that will be amortized over the different places where
HandleAllocator is also uses.

Taking a peek at what currently uses HandleAllocator, I see

image

in e.g. my AudioWorklet MOD player, https://clbri.com/modplayer/ , I'm not sure that it will ever use libfetch, libpromise, libwasmfs_opfs or libwebsocket. So it is not clear that this change will be a win.

The handle allocator itself is a bit contentious. It has a couple of challenges:

  1. If after this PR, one does
EMSCRIPTEN_WEBAUDIO_T context1 = emscripten_create_audio_context();
emscripten_destroy_audio_context(context1);

EMSCRIPTEN_WEBAUDIO_T context2 = emscripten_create_audio_context();
emscripten_audio_node_connect(someAudioNode, context1); // Programming error: incorrectly use context1 and not context2

Then the last line will not result in an error, but will silently connect the node to context2 instead. Imagine trying to debug a programming error where a subsystem keeps using a stale audio context in its operation. Accessing an invalid handle can acquire "phantom" meanings in the future.

  1. The HandleAllocator is unable to shrink memory, but retains a free list. E.g. temporarily having thousands of allocated handles, and then freeing them, will result in the free list keeping thousands of handles alive. (Imagine a native malloc() that would have a bookkeeping overhead of the max number of allocations ever made) To fix this drawback, the size of the HandleAllocator is likely to grow in the future, diminishing the supposed amortized code size win that is achieved here.

  2. Adding dependencies between libraries seems like a pessimization. In general one would aspire to move in the opposite direction - remove dependencies between systems. Whenever the common system is something complex, then the stronger coupling is warranted. But here it is just one JS map/dictionary {} implementation plus tracking a counter that is being shared across systems. That is not much. (and doesn't save on size)

Losing these would not be end of the world, but I am not sure what the upside of centralizing these dependencies is?

@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 26, 2025

Regarding your concerns (1) and (2), I completely agree. I do think we can and should address those. The nice thing about centralizing this mechanism is that when we do fix either (1) or (2) then all the subsystems that use the handle allocator will benefit. If we have N different allocation system then we cannot systematically address those kinds of issues.

Regarding (3), I'm not sure I agree, emscripten is full of shared abstractions and IMHO they have been a net win. e.g. callUserCallback, withBuiltinMalloc.

I do agree that (1) and (2) need to be address though, so maybe we can address them before landing this change since those concerns apply to all subsystems that use the HandleAllocator, right?

I just though one other nice thing that we can do once all subsystem share a common handle allocator: we can make handles globally unique (at least in debug builds). This was we can prevent even more types of programming errors: where handles from one API are mistakenly passed as handles into another API.. maybe less common but certainly good prevent type confusion.

sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 26, 2025
@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 26, 2025

Marking this as draft until (1) and (2) can be addressed.

The debugging changes I split out into #25655

@sbc100 sbc100 marked this pull request as draft October 26, 2025 21:37
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 26, 2025
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 26, 2025
@juj
Copy link
Collaborator

juj commented Oct 27, 2025

when we do fix either (1) or (2) then all the subsystems that use the handle allocator will benefit.

But if we don't share the code, then the issue that needs benefiting would not even arise.

emscripten is full of shared abstractions and IMHO they have been a net win. e.g. callUserCallback, withBuiltinMalloc.

That is true. I am not arguing that all shared abstractions are bad. But I am arguing that there should be actual substance to the sharing. Here with HandleAllocator, the sharing seems to be about abstracting over a JS {} dictionary, which is very little.

I just though one other nice thing that we can do once all subsystem share a common handle allocator: we can make handles globally unique (at least in debug builds).

It is quite rare for that to cause issues, e.g. passing a Web Audio context handle into a Fetch API call, is something that is really uncommon problem (as opposed to e.g. the use-after-free example above) to occur by a developer in the first place, and the browser JS API endpoints will croak about such issues anyways.

sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 27, 2025
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 27, 2025
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 27, 2025
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 27, 2025
sbc100 added a commit that referenced this pull request Oct 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants