Skip to content

Define memory in wasm module in MINIMAL_RUNTIME mode. #12323

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

Closed
wants to merge 2 commits into from

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Sep 24, 2020

See #12315

@sbc100 sbc100 requested review from kripken and juj September 24, 2020 02:32
@sbc100 sbc100 force-pushed the minimal_runtime_memory_export branch from d23aa01 to fff9ac2 Compare September 24, 2020 03:12
@sbc100 sbc100 changed the title Have wasm module export its memory in MINIMAL_RUNTIME mode. Define memory in wasm module in MINIMAL_RUNTIME mode. Sep 24, 2020
@sbc100 sbc100 force-pushed the minimal_runtime_memory_export branch 2 times, most recently from c143c22 to cb70ed2 Compare September 24, 2020 15:13
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Would it make sense to wait on this change for first deciding in general whether this direction is what we want?

@@ -195,3 +195,9 @@ var STRUCT_INFO = '';

// If true, building against Emscripten's asm.js/wasm heap memory profiler.
var MEMORYPROFILER = 0;

// By default we set this to 0 meaning the wasm file does not defined its own
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// By default we set this to 0 meaning the wasm file does not defined its own
// By default we set this to 0 meaning the wasm file does not define its own

@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 24, 2020

I'm happy to wait, especially since this change requires another binaryn change for wasm2js as well.

However I'm 99% this is what we want in the default case. The only question in mind right now is: do we need to continue to support the old way behind a flag?

Since table and memory are both created on the JS side the dynamic linking case it likely we have to continue to support this method anyway so the cost might be be much.

Also since its a code size win it seems like a must for MINIIMAL_RUNTIME (at least in the default case)

@kripken
Copy link
Member

kripken commented Sep 24, 2020

Oh, good point, since we need the option for dynamic linking anyhow, there is indeed no extra code complexity burden here.

@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 24, 2020

Oh, good point, since we need the option for dynamic linking anyhow, there is indeed no extra code complexity burden here.

I mean the code should be shared... but its still another config to test "static linking + memory supplied from the outside"

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

I think this is good, except for the setting - how about making it non-internal? I think that would be good for testing, for example, if someone hits a bug that only happens in standalone or minimal runtime mode (that flip this flag on by default) we can test in normal mode with the flag, to rule it out.

As we need to support these two modes anyhow, I don't see a significant downside to making it a user-facing setting.

@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 24, 2020

I'm trying to do this incrementally but eventually for normal mode I think we will want a flag.

For MINIMAL_RUNTIME and STANDALONE_WASM mode I'm not sure we want to support both ways as it complicates the code and neither of those modules support dynamic linking today.

Once that large change lands to allow this regular mode to support memory on the wasm side then I think we will have a flag at that point. But that change I imagine will be a fair bit larger and more complex than this one.. and more controversial perhaps.

This see this a small step towards that larger change, once that can land in relative isolation without exposing any new options or changing any existing behaviour.

@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 24, 2020

Anyway this is currently blocked on wasm2js so we need some binaryen changes first.

@sbc100 sbc100 force-pushed the minimal_runtime_memory_export branch 5 times, most recently from 6a6bc63 to 7fbcd1d Compare November 13, 2020 20:23
@sbc100 sbc100 force-pushed the minimal_runtime_memory_export branch 2 times, most recently from 1023fa7 to 67e6925 Compare November 15, 2020 17:28
@sbc100 sbc100 force-pushed the minimal_runtime_memory_export branch from 67e6925 to c651423 Compare November 15, 2020 17:29
@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 15, 2020

Abandoning in favor of going all the way and doing this by default with or without MINMAL_RUNTIME: #12790

@sbc100 sbc100 closed this Nov 15, 2020
@sbc100 sbc100 deleted the minimal_runtime_memory_export branch November 15, 2020 20: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.

3 participants