Skip to content

Clarify, rename, and FAQ memory allocation#288

Merged
lukewagner merged 5 commits intomasterfrom
refine-memory-allocation
Aug 13, 2015
Merged

Clarify, rename, and FAQ memory allocation#288
lukewagner merged 5 commits intomasterfrom
refine-memory-allocation

Conversation

@lukewagner
Copy link
Member

Based on conversations in #285 which seemed to have settled down, this PR clarifies how WebAssembly could (in MVP + future features) implement most of mmap, renames sbrk to something without a connotation, and adds an FAQ that explains why no noncontiguous allocation.

FAQ.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Fix link to #285.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@kripken
Copy link
Member

kripken commented Aug 6, 2015

I still believe it would be better to remove memory resizing from the MVP, since we know that we can't polyfill it well (@pizlonator for jsc, and last I heard v8 still had issues, but I'm not sure if that is still true - @titzer?).

I feel that the MVP is far more compelling when it just uses features we know polyfill and polyfill performantly. Since we plan to allow memory resizing after the MVP, this does not cause any long-term downsides - and people can still use asm.js if they need memory resizing in the interim; there are a few other features similarly limited in the MVP anyhow, for which we say that.

@lukewagner
Copy link
Member Author

First, I think it makes sense to require some module-level flag to opt-in to allowing heap-resizing. This allows various optimizations even with native wasm support. For the polyfill, this would control whether the changeHeap (resize) function was emitted. This would give developers the control of when the functionality was worth the hit.

As I understood it, the massive slowdown we saw in TurboAsm was not just because the singleton/closure analysis failed, but because the fallback was all the way back to ICs instead of the dynamic (hoisted) loads and length checks that were being emitted by CrankShaft. Has this since been fixed @titzer?

@pizlonator When you say in your comment "We support it, you just get punished for doing it. But your code will run.", I assume it's not the massive punishment mentioned above, but a fallback do dynamic loads of the heap base/length. Is that right?

@jfbastien
Copy link
Member

I'm not a fan of module-level flags, especially when we don't have dynamic linking and therefore have all the information up-front (list of module imports).

@lukewagner
Copy link
Member Author

I don't think we need an actual flag. Rather, based on what is currently in the design doc:

  • the main module gets to declare the min/max linear memory size
  • resize_memory calls (in any linked module) fail that would go above/below max/min
  • so disabling resize means having min = max

@jfbastien
Copy link
Member

"declaring" a value means there's magic in the platform that just "does the right thing". By doing this we're not giving the tools to developers to build things we don't anticipate, which goes against the Extensible Web's approach.

Agreed that sometimes implementation constraints mean that's what needs to be done, but I thought that we'd settled on exposing lower-level capabilities in this case?

@lukewagner
Copy link
Member Author

@jfbastien The min/max-size discussion really depends on what optimizations and impl techniques it allows. But that discussion should be in a separate issue since this PR is not adding it. Anyhow, still waiting to hear whether heap resizing in asm.js is a major slowdown in v8/jsc.

@lukewagner lukewagner force-pushed the refine-memory-allocation branch from 9a7b773 to fee4183 Compare August 12, 2015 18:23
@lukewagner
Copy link
Member Author

Rebased. Any other comments before merging?

@jfbastien
Copy link
Member

As we discussed I still think that the links aren't useful to this discussions: they're biased anecdotes, and explaining a fuller context wouldn't serve much of a purpose. I'd be happy with the patch minus the links.

@kripken
Copy link
Member

kripken commented Aug 12, 2015

My comment from before still stands about removing memory resizing from the MVP. Last I can see you were waiting for two people to respond, has there been further discussion there?

@lukewagner
Copy link
Member Author

I was still waiting for a response to my response and update to more narrowly scope the anecdotes.

@lukewagner
Copy link
Member Author

Resizing was already in the spec repo so I think the question of whether to remove it should be decoupled from all the issues being clarified here. An outcome of that discussion could be to move things from AstSemantics to FutureFeatures, although I hope that won't be necessary.

@kripken
Copy link
Member

kripken commented Aug 12, 2015

Fair enough, filed #294.

@lukewagner
Copy link
Member Author

Phrasing page_size as a global constant (which is quite unlike real user globals) was confusing, so rephrased as a nullary constant operation (so AST node) which makes more sense anyway.

@lukewagner
Copy link
Member Author

It's not worth holding up for some links in the FAQ, so removed and merged based on several lgtm.

lukewagner added a commit that referenced this pull request Aug 13, 2015
Clarify, rename, and FAQ memory allocation
@lukewagner lukewagner merged commit 9563c50 into master Aug 13, 2015
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.

4 participants