Use HandleAllocator for wasm workers. NFC

[sbc100]

Similar to what we have been doing to other APIs. For example, see #19337 and #19054

[juj]

I don't oppose if the code size is smaller, but to me it seems that this brings a dependency to a new object:

+n = new function() {
+    this.j = [ void 0 ];
+    this.m = [];
+    this.get = function(a) {
+        return this.j[a];
+    };
+    this.has = function(a) {
+        return void 0 !== this.j[a];
+    };
+    this.l = function(a) {
+        var c = this.m.pop() || this.j.length;
+        this.j[c] = a;
+        return c;
+    };
+};

This does not seem necessary, the existing code did not have any issues here that should be fixed?

HandleAllocator btw has an undesirable property of reusing the IDs it hands out. This can make debugging harder, since is not possible to detect dangling IDs. I.e. if one deletes a handle and then creates a new handle, that has the result of assigning the old deleted ID to the new object. If the user code had a bug of dangling IDs somewhere that were supposed to point to an old object, it will now suddenly jump to pointing to the new object, and there is no trace to being able to catch that happening.

For our WebGPU library, that was the main motivation of using a map {} instead of a list [], even if there might be a micro-performance win with JS arrays over maps, and WebGPU has a very high frequency of ID allocations necessary.

[tlively]

@sbc100 have chatted about the debugging story before as well. One option would be to swap out the implementation to avoid reusing handles only in non-release builds.

[tlively]

Is the test change related?

[sbc100]

This test is trying to try/catch around the use of an invalid handle. However, in the handle allocate we assert on invalid handle usage, which then calls abort. Sadly abort is not something that can be caught in the normal try/catch because it brings down the runtime before throwing, so you can't reasonably recover from it.

Perhaps you should be able to.. but perhaps not. Once a program calls abort I'm not sure we want it to be able to continue... so this test now has to a slightly more horrible hack of overriding the abort call itself.

[sbc100]

I don't oppose if the code size is smaller, but to me it seems that this brings a dependency to a new object:

I agree that code size will grow for certain users, but remember that this code is now shared with many of emscripten's subsystems so its highly like that the majority of programs out there will already be pulling the HandleAllocator. For all those users (I would imagine the majority of real-world usages) this is going to be code size win instead. So I would argue this could be a code size win in many cases.

Secondly I see several advantages of doing handle allocation in single share piece of code. We can build in debug support in a single location. We can amortize the cost of the code across the various subsystems. We can micro-optimize in a single place.. etc, etc.

[juj]

So I would argue this could be a code size win in many cases.

The links you posted referred to emval and fetch, I don't think all users are using wasm workers + emval + fetch, so the shared code size win argument does not seem to apply here.

Secondly I see several advantages of doing handle allocation in single share piece of code.

To me this reads the opposite: sharing this code (which is a trivial few lines overall) in multiple locations builds dependencies across multiple parts. And then in the future some user of this common code needs a new feature in it, and all users get bloated or slowed down. If it was a substantial common implementation of some nontrivial code, then this would make sense.

We can build in debug support in a single location.

.. but this change worsens debuggability, like I mentioned above? It also worsens debuggability by adding extra layer of indirection. After this change I might not be able to debug only wasm worker handle management, but I would print out also emval and fetch handles and all others? Overall, I am not sure either what the debuggability facilities here would be that we would need to build?

We can amortize the cost of the code across the various subsystems.

It does not seem that this sharing would be guaranteed occur in many codebases.

We can micro-optimize in a single place.. etc, etc.

The previous code already represented the micro-optimization? This seems like a deoptimization.

To me this kind of code represents the type of bloat creep of adding abstractions, which we worked hard to tear down. If it would be the case that we'd need to build some kind of super-robust handle machinery that is hard to do, then it might make sense.. but now this seems like abstracting over a single dictionary variable, which feels overkill.

[tlively]

Having a single handle management utility makes a lot of sense to me for all the reasons @sbc100 gave. If the previous code here had superior performance and debuggability, then that means that all the other users of HandleAllocator had inferior performance and debuggability. Since HandleAllocator is shared, we can improve it in one place and reap the benefits everywhere. If every subsystem had its own unique handle management code, that would not be possible.

[sbc100]

So I would argue this could be a code size win in many cases.

The links you posted referred to emval and fetch, I don't think all users are using wasm workers + emval + fetch, so the shared code size win argument does not seem to apply here.

Its not just embind, and fetch. Currently its used in:

• Fetch API
• WasmFS
• embind
• Promise API

And I'd like to soon to be used in:

• Old FS
• WebGL
• WebGPU
• WasmWorkers

Once its used in all those places it becomes more and more unlikely that any given program wouldn't include it.

[juj]

Its not just embind, and fetch. Currently its used in:

Gotcha, that makes more sense. However the code size increase here is something like 5x-10x the size of the old implementation, and the new implementation is worse in debuggability since it reuses the handles.

And I'd like to soon to be used in:
WebGL
WebGPU

If we would have an allocator that reuses handles in WebGL or WebGPU, then it is especially concerning. There it is a relatively common user programming error to have a dangling handle to e.g. a texture or a FBO, and then attempt to bind that handle to the GPU pipeline later. WebGL & WebGPU should definitely not use an implementation where it reuses handles, so that programmers get a clear error if they try to bind a handle that does not exist.

Once its used in all those places it becomes more and more unlikely that any given program wouldn't include it.

Could we go about this the way that we would prove the joint code size saving e.g. on a WasmWorkers+WebGL program, or a WasmFS+WebGL program? I understand that is maybe a bit tricky ask, but I put this out because I don't think the code size saving argument has weight here given the current implementation is so large compared to the trivial one.

Further, I am not convinced that different libraries necessarily will want to have the same implementation for a handle allocator, but different libraries might have different tradeoffs. Sharing the same code makes customization difficult.

The reason I am picky about this is that this handleallocator is wrapping over a trivial var map = {}; var runningIdCounter = 1; combo, which is remarkably small thing to wrap over. And the "wrapping over it will help debuggability" aspect of it does not seem plausible, especially given that the current change will take the debuggability to a worse place.

Ideally, if we have a common HandleAllocator, switching to it should fit like a glove in implementation. The only benefit proposed here is the ideality that same code is shared across places, but end users will not care about that ideality - they care about code size, debuggability (and performance, but that one is nonconsequential here since wasm worker instantiations are cold, only WebGL and WebGPU do high frequency handle allocs that might ever be observable). Also I think that we as maintainers do not practically need that ideality either, since it will not be a high frequency maintenance problem for different library.js files to have their own running ID counter mappings?

[sbc100]

OK, i'll put this one on the back burner for a while. At least until we are done converting webGL and webGPU and can prove not code size regressions (when in combination with those settings at least).