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

Refine linear memory aliasing #455

Merged
merged 1 commit into from
Nov 30, 2015
Merged

Refine linear memory aliasing #455

merged 1 commit into from
Nov 30, 2015

Conversation

lukewagner
Copy link
Member

This PR adds some more details to how linear memory is exported to JS from WebAssembly in Web.md to reflect more recent discussions concerning the hazards of resizing, mprotect and threads.

Heap exports also begs the question of heap imports which this PR takes the liberty of adding to FutureFeatures.md.

@lukewagner lukewagner added this to the MVP milestone Nov 9, 2015
@lukewagner
Copy link
Member Author

Oops, forgot about this. Any objections?

@titzer
Copy link

titzer commented Nov 25, 2015

lgtm

[Web](Web.md#aliasing-linear-memory-from-JS) would reflect exported linear
memory to JS as an `ArrayBuffer`. The MVP does not currently provide for
*importing* linear memory though this may be added
[in the future](FutureFeatures.md#importing-linear-memory).
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty wary of how this will interact with multi-threading and SAB. In C++ it's a bit of a fiasco to use shared memory:

  • volatile should be used to denote external modification, so the compiler remains conservative.
  • std::atomic should be used to enforce ordering and no-tear (yes, in addition to volatile).
  • Non-lock-free atomics are guaranteed to work in an address-free way within the same process but definitely do not work when using shared memory across processes.

This makes me wary of specifying things as you've outlined here without properly understanding what this implies for threads in WebAssembly. Does it entirely prevent optimizations? Valid reordering? 64-bit atomics on some platforms?

I'm OK specifying this as you've done fi you think it'll move stuff forward, but I insist on opening an issue with the above text, to revisit before MVP so we're not stuck when we want to add threads.

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm having trouble understanding your specific concern. At the top you mention wasm+SAB interactions but then the rest of your comment seems to be general memory model concerns that are not specific to SAB. There are some real concerns about resize+mprotect/SAB interactions outlined in Web.md, but those are quite different than what you're talking about here. Moreover, we have agreed that, because threads (not just SAB interactions) are such a cross-cutting concern, that we should have a good experimental prototype up and working before v.1 is finished; that's #104. So I'm not sure what specifically goes in a new issue.

Copy link
Member

Choose a reason for hiding this comment

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

The text reads as "oh I can just say externally alias and then everything works" but I see it as an undefined hole in the memory model. A hole we can plug, but will require some pretty serious design work that I'm not sure we want to tackle for MVP considering C++ hasn't gotten it right either.

That being said, maybe we can go with it for now, I can open an issue, and we can revisit when we have a clearer idea backed by code?

Copy link
Member Author

Choose a reason for hiding this comment

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

sgtm, happy to discuss more in that issue.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I opened #485.

lukewagner added a commit that referenced this pull request Nov 30, 2015
@lukewagner lukewagner merged commit b621477 into master Nov 30, 2015
@lukewagner lukewagner deleted the alias-from-js branch November 30, 2015 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants