-
-
Notifications
You must be signed in to change notification settings - Fork 675
Do not allocate MemorySegments for memory.data(i32, i32?) #2831
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
Conversation
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.
Would this perhaps become simpler[...]?
Oh, that's...a much better idea. Thanks and sorry about that.
373166c to
6a89a09
Compare
Might well be a non-issue in practice, though.
@dcodeIO hmm, would this conflict when --importMemory is specified without --zeroFilledMemory?
ed50aef to
4465767
Compare
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.
4465767 to
163a631
Compare
There was a problem hiding this 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.
@HerrCai0907 I changed the commit message; this is technically a breaking change.
@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.
Fixes #2827.
Changes proposed in this pull request:
⯈ Bump
Compiler#memoryOffsetinstead of allocating zero-filled segments.There is an issue with the
initialPagescalculation ininitDefaultMemory, however. The existing condition to run that calculation depends onmemorySegments.length, which is now zero if onlymemory.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, or8, modules that previously compiled with an initial memory of 0 pages would compile with a minimum of 1 page.