Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CountBleck
Copy link
Member

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.

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

src/compiler.ts Outdated Show resolved Hide resolved
@dcodeIO
Copy link
Member

dcodeIO commented Mar 25, 2024

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
Copy link
Member Author

Would this perhaps become simpler[...]?

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

@CountBleck CountBleck force-pushed the fix-large-memory-crash branch from 373166c to 6a89a09 Compare March 25, 2024 01:49
@CountBleck
Copy link
Member Author

CountBleck commented Mar 25, 2024

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

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

@CountBleck CountBleck marked this pull request as ready for review March 25, 2024 03:00
src/module.ts Show resolved Hide resolved
@CountBleck CountBleck force-pushed the fix-large-memory-crash branch 3 times, most recently from ed50aef to 4465767 Compare April 3, 2024 01:46
MemorySegments can now hold null buffers, which signifies that memory is
in fact used in the compiled program, for programs that only reserve
zero-filled memories. Using null buffers avoids passing zero-filled
buffers to Binaryen, which isn't necessary since Wasm memories are
initialized with zeroes when a (data ...) segment isn't present.

This change is breaking in that modules using imported memories that
contain non-zero values in such memory ranges at the time of
instantiation will retain those values, as opposed to being filled.
This shouldn't affect most users, however, and it could possibly be
desirable.

Fixes AssemblyScript#2827.
@CountBleck CountBleck force-pushed the fix-large-memory-crash branch from 4465767 to 163a631 Compare April 3, 2024 01:47
@CountBleck CountBleck requested a review from HerrCai0907 April 3, 2024 01:47
Copy link
Member

@HerrCai0907 HerrCai0907 left a comment

Choose a reason for hiding this comment

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

LGTM.
A tip: Please avoid to force push after someone has already reviewed PR. This makes it difficult for reviewers to track.

@CountBleck
Copy link
Member Author

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

@HerrCai0907
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

as crash when try to alloc large data
3 participants