Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 2, 2023

If this is needed it can always be found via HEAPXX, or wasmMemory.

Also introduce a new debug helper called missingGlobal which allows
us to remove builtin global symbols while notifying any folks that
are depending on them.

@sbc100 sbc100 force-pushed the simplify_init_mem branch from 6b41fb1 to c88c687 Compare January 4, 2023 17:20
@sbc100 sbc100 force-pushed the remove_buffer branch 2 times, most recently from c483bdf to 9861cc4 Compare January 4, 2023 17:33
@sbc100 sbc100 force-pushed the simplify_init_mem branch from c88c687 to 9dafe9b Compare January 4, 2023 18:48
Base automatically changed from simplify_init_mem to main January 4, 2023 23:50
@sbc100 sbc100 changed the title WIP: Remove undeeded buffer global Remove undeeded buffer global Jan 4, 2023
@sbc100 sbc100 requested a review from kripken January 5, 2023 00:00
@sbc100 sbc100 changed the title Remove undeeded buffer global Remove unneeded buffer global Jan 5, 2023
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it might add a few bytes, but the simplification seems worth it.

(Removing the parameter should have decreased code size, but maybe closure was doing that all along anyhow?)

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 5, 2023

Looks like it might add a few bytes, but the simplification seems worth it.

(Removing the parameter should have decreased code size, but maybe closure was doing that all along anyhow?)

Yes, it only adds bytes in non-optimizing builds, but I also think it make non-optimized builds more readable.

For optimized builds its a wash (I assume because of closure).

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 5, 2023

I'm going to add a ChangeLog entry for this I think

If this is needed it can always be found via HEAPXX, or wasmMemory.

Also introduce a new debug helper called `missingGlobal` which allows
use to remove builtin global symbols while notifying any folks that
are depending on them.
@sbc100 sbc100 merged commit 72f5fb1 into main Jan 5, 2023
@sbc100 sbc100 deleted the remove_buffer branch January 5, 2023 19:01
juj added a commit to juj/wasm_webgpu that referenced this pull request Jan 27, 2023
@juj
Copy link
Collaborator

juj commented Oct 31, 2023

This regressed Unity's end user at https://forum.unity.com/threads/arrays-shared-between-c-and-jslib-in-webgl-buffer-undefined-in-2023-3-0a6.1495094/ .

In Emscripten documentation, I see there is one location that still looks like is referencing buffer:

site\source\docs\optimizing\Module-Splitting.rst:

Here’s code implementing the base64 solution::

  var profile_data = new Uint8Array(buffer, ptr, len);

Also, we still have the following

site\source\docs\api_reference\module.rst:

.. js:attribute:: Module.buffer

  Allows you to provide your own ``ArrayBuffer`` or ``SharedArrayBuffer`` to use as the memory.

  .. note:: This is only supported if ``-sWASM=0``. See ``Module.wasmMemory`` for WebAssembly support.

site\source\docs\compiling\Deploying-Pages.rst:

See the fields ``Module['buffer']`` and ``Module['wasmMemory']`` for more information.

is the intent to get rid of Module['buffer'] as well? (not that the JS scope and Module objects would generally be symmetric, but just to be sure)

In terms of code size, this PR is a pessimization. I think the code size comparison above was not apples-to-apples, due to also removing the function parameter in the PR.

In small test programs, a dedicated let buffer = wasmMemory.buffer; variable assignment would dominate (minifies down to ,b=w.buffer), but in larger programs, the number of references to wasmMemory.buffer or HEAP8.buffer will all weigh +2 bytes compared to before this change, so will overtake the constant overhead at some point.

@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 31, 2023

I don't think this this change effected Module['buffer']. I don't think that was available before this change either. From the docs it looks like maybe it was pre-wasm thing?

Looking at the codebase it doesn't look like references to wasmMemory.buffer or HEAP8.buffer should be very common. Do you know why the user in question was depending on it? i.e. what were they looking to do with buffer? I assume this reference was in libary-js code?

@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 31, 2023

The readable Module['buffer'] was removed in #8277.

Module['buffer'] and Module['wasmMemory'] can still be used as part of the incoming module API which is what those docs are referring too I believe.

sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 31, 2023
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.

4 participants