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

Simplfy memory growth code for polyfill interpreter #5173

Closed
wants to merge 1 commit into from

Conversation

dschuff
Copy link
Member

@dschuff dschuff commented Apr 28, 2017

This will allow us to remove the duplicate code in wasm-js.cpp in Binaryen.

This will allow us to remove the duplicate code in wasm-js.cpp in Binaryen.
@dschuff
Copy link
Member Author

dschuff commented Apr 28, 2017

Unfortunately I don't see a good way to get rid of the call into __growWasmMemory entirely. I think this is because the interpreter must have its own representation of the linear memory and doesn't really support importing memory?

@@ -2381,22 +2381,29 @@ function integrateWasmJS(Module) {
if (Module["usingWasm"]) {
try {
var result = Module['wasmMemory'].grow((size - oldSize) / wasmPageSize); // .grow() takes a delta compared to the previous size
if (result !== (-1 | 0)) {
Copy link
Member

Choose a reason for hiding this comment

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

don't we still need this code? it checks if grow failed.

exports['__growWasmMemory']((size - oldSize) / wasmPageSize); // tiny wasm method that just does grow_memory
// in interpreter, we replace Module.buffer if we allocate
return Module['buffer'] !== old ? Module['buffer'] : null; // if it was reallocated, it changed
// This path is only used by the polyfill interpreter.
Copy link
Member

Choose a reason for hiding this comment

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

i don't understand what this change does?

@dschuff dschuff closed this Jul 2, 2019
@dschuff dschuff deleted the interpret_memorygrowth branch July 2, 2019 22:09
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.

None yet

2 participants