AssemblyScript / assemblyscript

A TypeScript-like language for WebAssembly.
https://www.assemblyscript.org
Apache License 2.0
16.82k stars 655 forks source link

Do not allocate MemorySegments for memory.data(i32, i32?) #2831

Open CountBleck opened 6 months ago

CountBleck commented 6 months ago

Fixes #2827.

Changes proposed in this pull request: ⯈ Bump Compiler#memoryOffset instead of allocating zero-filled segments.

There is an issue with the initialPages calculation in initDefaultMemory, however. The existing condition to run that calculation depends on memorySegments.length, which is now zero if only memory.data(n: i32) is called, as in the memory-config-errors test. Making that calculation occur regardless of the number of memory segments would cause a separate issue: due to the default memory offsets of options.memoryBase, 1024, or 8, modules that previously compiled with an initial memory of 0 pages would compile with a minimum of 1 page.

dcodeIO commented 6 months ago

Would this perhaps become simpler if we'd keep tracking such segments as MemorySegments but allowed the buffer to be null, indicating all zeroes?

More generally, when not indicating the presence of the segment to Binaryen, we'd always have to assume that memory is zeroed, as the zeroes won't be in the (memory ...). Might well be a non-issue in practice, though.

CountBleck commented 6 months ago

Would this perhaps become simpler[...]?

Oh, that's...a much better idea. Thanks and sorry about that.

CountBleck commented 6 months ago

Might well be a non-issue in practice, though.

@dcodeIO hmm, would this conflict when --importMemory is specified without --zeroFilledMemory?

CountBleck commented 6 months ago

@HerrCai0907 I changed the commit message; this is technically a breaking change.

HerrCai0907 commented 6 months ago

@HerrCai0907 I changed the commit message; this is technically a breaking change.

You can modify it when click squash and merge😆 . Actually the message in commit is meaningless.