GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Do not allocate MemorySegments for memory.data(i32, i32?) by CountBleck · Pull Request #2831 · AssemblyScript/assemblyscript

Conversation

@CountBleck

@CountBleck CountBleck commented Mar 25, 2024

Copy link
Copy Markdown
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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
CountBleck
CountBleck commented Mar 25, 2024
View reviewed changes
Comment thread src/compiler.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.
@dcodeIO

dcodeIO commented Mar 25, 2024

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@CountBleck

CountBleck commented Mar 25, 2024

Copy link
Copy Markdown
Member Author

Would this perhaps become simpler[...]?

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@CountBleck
CountBleck force-pushed the fix-large-memory-crash branch from 373166c to 6a89a09 Compare March 25, 2024 01:49
@CountBleck

CountBleck commented Mar 25, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@CountBleck
CountBleck requested review from HerrCai0907 and dcodeIO March 25, 2024 01:53
@CountBleck
CountBleck marked this pull request as ready for review March 25, 2024 03:00
HerrCai0907
HerrCai0907 approved these changes Mar 28, 2024
View reviewed changes
Comment thread src/module.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.
@CountBleck
CountBleck force-pushed the fix-large-memory-crash branch 3 times, most recently from ed50aef to 4465767 Compare April 3, 2024 01:46
@CountBleck
BREAKING CHANGE: Do not allocate buffers for memory.data(i32, i32?)
163a631 
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
HerrCai0907
HerrCai0907 approved these changes Apr 3, 2024
View reviewed changes

@HerrCai0907 HerrCai0907 left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@CountBleck

CountBleck commented Apr 3, 2024

Copy link
Copy Markdown
Member Author

@HerrCai0907 I changed the commit message; this is technically a breaking change.
romdotdog and msqr1 reacted with thumbs down emoji
All reactions
  • 👎 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@CountBleck CountBleck added the breaking change label Apr 3, 2024
@HerrCai0907

HerrCai0907 commented Apr 3, 2024

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dcodeIO dcodeIO Awaiting requested review from dcodeIO
1 more reviewer
@HerrCai0907 HerrCai0907 HerrCai0907 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

breaking change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

as crash when try to alloc large data

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@CountBleck @dcodeIO @HerrCai0907
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.