emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.62k stars 3.28k forks source link

--separate-asm + -s BINARYEN=1 still downloads .asm.js code #4909

Closed juj closed 5 years ago

juj commented 7 years ago

Running em++ tests\hello_world.cpp --separate-asm -s BINARYEN=1 -o a.html, gives the following in the generated a.html file:

      var xhr = new XMLHttpRequest();
      xhr.open('GET', 'a.wasm', true);
      xhr.responseType = 'arraybuffer';
      xhr.onload = function() {
        Module.wasmBinary = xhr.response;

var script = document.createElement('script');
script.src = "a.asm.js";
script.onload = function() {
  setTimeout(function() {

      var script = document.createElement('script');
      script.src = "a.js";
      document.body.appendChild(script);

  }, 1); // delaying even 1ms is enough to allow compilation memory to be reclaimed
};
document.body.appendChild(script);

      };
      xhr.send(null);

which downloads .asm.js even when generating .asm.js was not requested. The extra code here is

      var xhr = new XMLHttpRequest();
      xhr.open('GET', 'a.wasm', true);
      xhr.responseType = 'arraybuffer';
      xhr.onload = function() {
        Module.wasmBinary = xhr.response;

//var script = document.createElement('script');
//script.src = "a.asm.js";
//script.onload = function() {
//  setTimeout(function() {

      var script = document.createElement('script');
      script.src = "a.js";
      document.body.appendChild(script);

//  }, 1); // delaying even 1ms is enough to allow compilation memory to be reclaimed
//};
//document.body.appendChild(script);

      };
      xhr.send(null);

The solution is to drop the --separate-asm flag here, which removes the extra code.

This hints that perhaps we should make --separate-asm flag be ignored when targeting only Wasm, since it generates this redundant code?

juj commented 7 years ago

Related, building with -s BINARYEN_METHOD='native-wasm,asmjs' implies --separate-asm, since 'native-wasm,asmjs' architecture requires that .asm.js file is stored separately.

However if one does pass --separate-asm explicitly with -s BINARYEN_METHOD='native-wasm,asmjs', it makes things worse by unconditionally downloading both files, like shown above. So I suppose when -s BINARYEN=1, we should silently ignore --separate-asm.

kripken commented 7 years ago

Well, if the asm.js might be used - like when the method is native-wasm,asmjs - then we do need to load the asm.js. So unconditionally preloading both wasm and asm.js in that case is right, I think.

Maybe we could just add a warning if --separate-asm is passed but the asm.js is not going to be used (not in the METHOD)? And if we don't already, document that --separate-asm will load the asm.js for you (when generating html).

juj commented 7 years ago

I think we should never unconditionally download both, but first figure out which one would be needed, and only download that one? The above PR does seem to work well at in at least avoiding the .asm.js download at startup, does that make sense to you? Although I wonder if I'm seeing this right and it's now doing a sync XHR to obtain it when wasm support is disabled, hmm.

kripken commented 7 years ago

Well, that's assuming we can never be surprised later. E.g. you might see the browser has wasm support, but when you try to instantiate the wasm, it might fail for some reason. I guess the question is whether we want to support that. Currently, binaryen tries each method and checks for failure, it doesn't assume it can know ahead of time which will work (if it could know that, it wouldn't need to check one by one).

How important is it to optimize the case of fallback/multiple methods? Any build with both asm and wasm enabled will be a poorly-optimized one anyhow.

juj commented 7 years ago

Ideally we would in such a scenario first download .wasm, try it, if it doesn't work, only then download .asm.js.

Supporting fallback is important, at least for a while, but at present the -s BINARYEN_METHOD=native-wasm,asmjs is kind of unusable because of the asm.js coming out slow, that we ask people to build twice and manually develop the .html file to do the fallback they want.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because there has been no activity in the past 2 years. It will be closed automatically if no further activity occurs in the next 7 days. Feel free to re-open at any time if this issue is still relevant.