Create memory in wasm by default #12790

sbc100 · 2020-11-15T19:18:59Z

There are some cases where we want to create the memory in JS but for
most programs exporting the memory from wasm makes more sense and is
more compact and reliable.

The new EXTERNAL_MEMORY setting can be used to override this behaviour.

juj · 2020-11-15T20:24:30Z

src/preamble_minimal.js

+if (ENVIRONMENT_IS_WORKER) {
+  updateGlobalBufferAndViews({{{EXPORT_NAME}}}.buffer);
+} else {
+  updateGlobalBufferAndViews(wasmMemory.buffer);


This if (ENVIRONMENT_IS_WORKER) looks odd and like a code size regression? Is this intended to be the case for pthread local thread init? Then would the following work?

#if USE_PTHREADS updateGlobalBufferAndViews({{{EXPORT_NAME}}}.buffer || wasmMemory.buffer); #else updateGlobalBufferAndViews(wasmMemory.buffer); #endif

Oh now I see it comes from the moved block below. Lgtm. (looks like there is room to optimize this later)

juj · 2020-11-15T20:27:19Z

src/postamble_minimal.js

+#endif
+
+#if !EXTERNAL_MEMORY
+  wasmMemory = asm['memory'];


Does this mean that users cannot implement a function named memory in their programs? I would recommend renaming this to __mem.

I do agree in general, and there as been discussion about this in upstream llvm in the past.

The problem is that the name of the memory import/export is determined my wasm-ld. We have the --import-memory and --export-memory flags but the name is currently fixed. The name __indirect_function_table was chosen specifically to avoid such collisions. IIRC we tried to do this in the past but got push back because it would break existing users.

BTW, this doesn't effect all users who have functions called memory, it only effects users who have a function called memory who also want to include it in EXPORTED_FUNCTIONS. If the former was true I would be far more worried about this concern.

That is indeed true.. still feels a bit sloppy I must admit, would be nice to be strict about namespacing :/

Also not that with the current situation we already reserve the name memory for the export from JS to WebAssembly.. so users today are unable to export JS functions called "memory". This change reversed that existing restriction and its not clear to make it makes the situation any worse or better.

We should attempt to re-name memory but I think that goal is mostly orthogonal to this change.

juj · 2020-11-15T20:29:56Z

Great feature!

The new EXPORT_MEMORY setting can be used to override this behaviour.

Looks like the code uses a name EXTERNAL_MEMORY instead. I think EXTERNAL is better here than EXPORT though.

src/settings.js

kripken

Nice work!

kripken · 2020-11-17T03:12:35Z

src/postamble.js

@@ -8,11 +8,12 @@

 {{{ exportRuntime() }}}

-#if MEM_INIT_IN_WASM == 0
+#if !MEM_INIT_IN_WASM
+function runMemoryInitializer() {


I'm not sure how to read this code. This new function starts here, but the PR doesn't seem to end it? Maybe I'm missing something.

It looks funny because I create new function, but removed the internal nesting within the function, replacing it with an early return.

So the overall level of nesting remained constant even though a function was added.

src/preamble.js

src/runtime_init_memory.js

kripken · 2020-11-17T03:17:07Z

tests/code_size/hello_world_wasm2js.json

@@ -1,10 +1,10 @@
 {
  "a.html": 697,
  "a.html.gz": 437,
-  "a.js": 865,
-  "a.js.gz": 503,
+  "a.js": 1131,


some of these sizes seem larger, do you know why?

I think its only the wasm2js tests that show increases. I guess the cost of creating the "fake" memory in wasm2js is at least as big or little bigger than the savings we get from not creating the memory in the normal JS.

Perhaps there is some way to combine the fake WebAssembly memory in src/wasm2js.js with the fake one that wasm2js in binaryen generates to save some bytes?

I think we can take a look at shrinking the overall wasm2js size as a followup if we think its worth it. (Or we can default to IMPORTED_MEMORY for wasm2js?

It is interesting that this particular wasm2js test regressed more than any other from some reason. The other wasm2js regressions are only a around ~10 bytes.

It looks like this is probably with investigating. Somehow in this one case some of the JS optimizations are being defeated.

I've attached the a.js file before and after this change. Maybe you can see what it is? I suspect its AJSDCE

Thanks, reading the outputs, looks like two issues here.

One may indeed be in JSDCE. It should remove new Int8Array(m); (without an assignment to anywhere - technically it has side effects, but only of potentially throwing on an invalid input or out of memory). This code should be doing it, so some debug prints might help there:

emscripten/tools/acorn-optimizer.js

Lines 250 to 263 in 574bce3

case 'NewExpression': {

// default to unsafe, but can be safe on some familiar objects

if (node.callee.type === 'Identifier') {

var name = node.callee.name;

if (name === 'TextDecoder' || name === 'ArrayBuffer' ||

name === 'Int8Array' || name === 'Uint8Array' ||

name === 'Int16Array' || name === 'Uint16Array' ||

name === 'Int32Array' || name === 'Uint32Array' ||

name === 'Float32Array' || name === 'Float64Array') {

// no side effects, but the arguments might (we walk them in

// full walk as well)

break;

}

}

The second issue is that we now have

var b = new Int8Array(a); var c = new Int16Array(a); var d = new Int32Array(a); var e = new Uint8Array(a); var f = new Uint16Array(a); var g = new Uint32Array(a); var h = new Float32Array(a); var i = new Float64Array(a);

inside the wasm2js output. That can't be removed by JSDCE, which doesn't look all the way in there. Maybe wasm2js itself can avoid emitting unnecessary ones, although that would require some tracking of things, and might not be easy. It's possible that fixing the previous issue would be enough to avoid a regression here, so this might not be worth it (but could be a TODO).

So actually it looks like this is really the thing that makes all the difference here:

+ var a = new ArrayBuffer(16777216); + var b = new Int8Array(a); + var c = new Int16Array(a); + var d = new Int32Array(a); + var e = new Uint8Array(a); + var f = new Uint16Array(a); + var g = new Uint32Array(a); + var h = new Float32Array(a); + var i = new Float64Array(a);

For some reason I can no longer reproduce the stray new Int8Array(m); at the top level.. so I think this change is good to land and we can look at cleaning up the wasm2js boilerplat at the top of the asmFunc somehow has a followup.

I'm not opposed to that, but assuming we have a plan for that followup. Just guessing at it, I worry it might not be practical to thread the dependencies everywhere in wasm2js, but hopefully it's actually easier when looking at it.

From @juj's response here (#12806 (comment)) it sounds like this is something we are already suffering from can clusure should really be able to clean up this stuff for us.

Any idea why its not?

Should I default to IMPORTED_MEMORY in wasm2js mode until this regression is fixed?

I added WASM2JS to the list of exclusions from this feature for now.

I mean WASM2JS

sbc100 · 2020-11-17T04:37:12Z

js.zip

ChangeLog.md

kripken · 2020-11-17T22:29:03Z

tests/code_size/hello_world_wasm2js.json

@@ -1,10 +1,10 @@
 {
  "a.html": 697,
  "a.html.gz": 437,
-  "a.js": 865,
-  "a.js.gz": 503,
+  "a.js": 1131,


I'm not opposed to that, but assuming we have a plan for that followup. Just guessing at it, I worry it might not be practical to thread the dependencies everywhere in wasm2js, but hopefully it's actually easier when looking at it.

There are some cases where we want to create the memory in JS but for most programs exporting the memory from wasm makes more sense and is more compact and reliable. The new EXTERNAL_MEMORY setting can be used to override this behaviour.

All this setting was doing was setting MALLOC=none so it doesn't seem like it needs to exist as a separate setting. I think it used to do more, for example, prior to #12790 it used was used in some JS library code. Another PR that removed a lot of usage of this settings was #12057. Simply removing it simplifies another change I'm working on: #18905. Also, the docs were incorrect in stating it was ignored except under MINIMAL_RUNTIME, but we had no testing for it.

sbc100 changed the title ~~Create memory in wasm binaryn by default~~ Create memory in wasm by default Nov 15, 2020

sbc100 force-pushed the memory_defined_in_wasm branch from 76f7136 to f5c37c0 Compare November 15, 2020 19:58

sbc100 mentioned this pull request Nov 15, 2020

Define memory in wasm module in MINIMAL_RUNTIME mode. #12323

Closed

sbc100 force-pushed the memory_defined_in_wasm branch 2 times, most recently from 9d79cc4 to 83dd2fb Compare November 15, 2020 20:14

juj reviewed Nov 15, 2020

View reviewed changes

kripken reviewed Nov 15, 2020

View reviewed changes

src/settings.js Outdated Show resolved Hide resolved

sbc100 force-pushed the memory_defined_in_wasm branch 2 times, most recently from a7d7a69 to 2fc7346 Compare November 16, 2020 18:25

kripken reviewed Nov 17, 2020

View reviewed changes

sbc100 force-pushed the memory_defined_in_wasm branch from 542d672 to 646ebc5 Compare November 17, 2020 03:53

kripken approved these changes Nov 17, 2020

View reviewed changes

sbc100 force-pushed the memory_defined_in_wasm branch 3 times, most recently from 3c2de02 to cb7c2cd Compare November 18, 2020 01:15

Create memory in wasm binaryn by default

a3b9b2d

There are some cases where we want to create the memory in JS but for most programs exporting the memory from wasm makes more sense and is more compact and reliable. The new EXTERNAL_MEMORY setting can be used to override this behaviour.

sbc100 force-pushed the memory_defined_in_wasm branch from cb7c2cd to a3b9b2d Compare November 18, 2020 01:38

sbc100 merged commit c2462cd into master Nov 18, 2020

sbc100 deleted the memory_defined_in_wasm branch November 18, 2020 03:06

sbc100 mentioned this pull request Apr 12, 2023

Deprecate the USES_DYNAMIC_ALLOC setting #19164

Merged

	case 'NewExpression': {
	// default to unsafe, but can be safe on some familiar objects
	if (node.callee.type === 'Identifier') {
	var name = node.callee.name;
	if (name === 'TextDecoder' \|\| name === 'ArrayBuffer' \|\|
	name === 'Int8Array' \|\| name === 'Uint8Array' \|\|
	name === 'Int16Array' \|\| name === 'Uint16Array' \|\|
	name === 'Int32Array' \|\| name === 'Uint32Array' \|\|
	name === 'Float32Array' \|\| name === 'Float64Array') {
	// no side effects, but the arguments might (we walk them in
	// full walk as well)
	break;
	}
	}

Create memory in wasm by default #12790

Create memory in wasm by default #12790

Uh oh!

Conversation

sbc100 commented Nov 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juj commented Nov 15, 2020

Uh oh!

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Nov 17, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbc100 commented Nov 15, 2020 •

edited

Loading

sbc100 Nov 17, 2020 •

edited

Loading