Make file packager --no-heap-copy always apply, and remove the option (#12027)

The old default was to copy a file package into main memory (into space
malloc'd for it). That was not memory-efficient as it meant the main memory
needed to be bigger, and also the memory could never be freed. While
mmap on that could be fast, we added the --no-heap-copy option to
keep file data on the JS heap and not copy it in. That uses less memory
but still is just as fast at fread etc.

This makes that newer option the default, and removes the option to do it
the old way. This is better for memory usage, and in any case people using
file data in a heavy way may be rolling their own solutions anyhow.

This also removes the file system usage of getMemory(), which would
perform allocation during startup from JS, which is a source of complexity
I am working to remove for WebAssembly/binaryen#3043

Author: kripken

ChangeLog.md

@@ -17,6 +17,9 @@ See docs/process.md for how version tagging works.
 
 Current Trunk
 -------------
+- Enable `--no-heap-copy` file packager option by default, and remove the old
+  defualt behavior entirely. That is the behavior we should have had from the
+  beginning as it is more memory-efficient.
 - `--no-entry` is now required in `STANDALONE_WASM` mode when building a reactor
   (application without a main function).  Previously exporting a list of
   functions that didn't include `_main` would imply this.  Now the list of

emcc.py

@@ -243,7 +243,6 @@ def __init__(self):
     self.memory_profiler = False
     self.memory_init_file = None
     self.use_preload_cache = False
-    self.no_heap_copy = False
     self.use_preload_plugins = False
     self.proxy_to_worker = False
     self.default_object_extension = '.o'
@@ -2112,13 +2111,6 @@ def get_final():
     with ToolchainProfiler.profile_block('source transforms'):
       # Embed and preload files
       if len(options.preload_files) or len(options.embed_files):
-
-        # Also, MEMFS is not aware of heap resizing feature in wasm, so if MEMFS and memory growth are used together, force
-        # no_heap_copy to be enabled.
-        if shared.Settings.ALLOW_MEMORY_GROWTH and not options.no_heap_copy:
-          logger.info('Enabling --no-heap-copy because -s ALLOW_MEMORY_GROWTH=1 is being used with file_packager.py (pass --no-heap-copy to suppress this notification)')
-          options.no_heap_copy = True
-
         logger.debug('setting up files')
         file_args = ['--from-emcc', '--export-name=' + shared.Settings.EXPORT_NAME]
         if len(options.preload_files):
@@ -2132,8 +2124,6 @@ def get_final():
           file_args += options.exclude_files
         if options.use_preload_cache:
           file_args.append('--use-preload-cache')
-        if options.no_heap_copy:
-          file_args.append('--no-heap-copy')
         if shared.Settings.LZ4:
           file_args.append('--lz4')
         if options.use_preload_plugins:
@@ -2420,7 +2410,7 @@ def consume_arg():
       options.use_preload_cache = True
       newargs[i] = ''
     elif newargs[i].startswith('--no-heap-copy'):
-      options.no_heap_copy = True
+      diagnostics.warning('legacy-settings', 'ignoring legacy flag --no-heap-copy (that is the only mode supported now)')
       newargs[i] = ''
     elif newargs[i].startswith('--use-preload-plugins'):
       options.use_preload_plugins = True

src/library_memfs.js

@@ -289,15 +289,6 @@ mergeInto(LibraryManager.library, {
         // memory buffer, as they may get invalidated. That means we
         // need to do copy its contents.
         if (buffer.buffer === HEAP8.buffer) {
-#if ASSERTIONS
-          // FIXME: this is inefficient as the file packager may have
-          //        copied the data into memory already - we may want to
-          //        integrate more there and let the file packager loading
-          //        code be able to query if memory growth is on or off.
-          if (canOwn) {
-            warnOnce('file packager has copied file data into memory, but in memory growth we are forced to copy it again (see --no-heap-copy)');
-          }
-#endif // ASSERTIONS
           canOwn = false;
         }
 #endif // ALLOW_MEMORY_GROWTH

tests/test_browser.py

@@ -257,7 +257,6 @@ def make_main(path):
       make_main(dstpath)
       self.compile_btest(['main.cpp', '--preload-file', srcpath, '-o', 'page.html'])
       self.run_browser('page.html', 'You should see |load me right before|.', '/report_result?1')
-    # Test that '--no-heap-copy' works.
     if WINDOWS:
       # On Windows, the following non-alphanumeric non-control code ASCII characters are supported.
       # The characters <, >, ", |, ?, * are not allowed, because the Windows filesystem doesn't support those.
@@ -268,7 +267,7 @@ def make_main(path):
     open(os.path.join(self.get_dir(), tricky_filename), 'w').write('''load me right before running the code please''')
     make_main(tricky_filename)
     # As an Emscripten-specific feature, the character '@' must be escaped in the form '@@' to not confuse with the 'src@dst' notation.
-    self.compile_btest(['main.cpp', '--preload-file', tricky_filename.replace('@', '@@'), '--no-heap-copy', '-o', 'page.html'])
+    self.compile_btest(['main.cpp', '--preload-file', tricky_filename.replace('@', '@@'), '-o', 'page.html'])
     self.run_browser('page.html', 'You should see |load me right before|.', '/report_result?1')
 
     # By absolute path
@@ -2435,8 +2434,7 @@ def test_preload_module(self):
 
   def test_mmap_file(self):
     create_test_file('data.dat', 'data from the file ' + ('.' * 9000))
-    for extra_args in [[], ['--no-heap-copy']]:
-      self.btest(path_from_root('tests', 'mmap_file.c'), expected='1', args=['--preload-file', 'data.dat'] + extra_args)
+    self.btest(path_from_root('tests', 'mmap_file.c'), expected='1', args=['--preload-file', 'data.dat'])
 
   def test_emrun_info(self):
     if not has_browser():
@@ -2819,36 +2817,35 @@ def test_sdl2_image_formats(self):
                                                      '-DBITSPERPIXEL=24', '-DNO_PRELOADED', '-s', 'USE_SDL=2', '-s', 'USE_SDL_IMAGE=2', '-s', 'SDL2_IMAGE_FORMATS=["jpg"]'])
 
   def test_sdl2_key(self):
-    for defines in [[]]:
-      create_test_file('pre.js', '''
-        Module.postRun = function() {
-          function doOne() {
-            Module._one();
-            setTimeout(doOne, 1000/60);
-          }
+    create_test_file('pre.js', '''
+      Module.postRun = function() {
+        function doOne() {
+          Module._one();
           setTimeout(doOne, 1000/60);
         }
+        setTimeout(doOne, 1000/60);
+      }
 
-        function keydown(c) {
-          var event = new KeyboardEvent("keydown", { 'keyCode': c, 'charCode': c, 'view': window, 'bubbles': true, 'cancelable': true });
-          var prevented = !document.dispatchEvent(event);
-
-          //send keypress if not prevented
-          if (!prevented) {
-            var event = new KeyboardEvent("keypress", { 'keyCode': c, 'charCode': c, 'view': window, 'bubbles': true, 'cancelable': true });
-            document.dispatchEvent(event);
-          }
-        }
+      function keydown(c) {
+        var event = new KeyboardEvent("keydown", { 'keyCode': c, 'charCode': c, 'view': window, 'bubbles': true, 'cancelable': true });
+        var prevented = !document.dispatchEvent(event);
 
-        function keyup(c) {
-          var event = new KeyboardEvent("keyup", { 'keyCode': c, 'charCode': c, 'view': window, 'bubbles': true, 'cancelable': true });
+        //send keypress if not prevented
+        if (!prevented) {
+          var event = new KeyboardEvent("keypress", { 'keyCode': c, 'charCode': c, 'view': window, 'bubbles': true, 'cancelable': true });
           document.dispatchEvent(event);
         }
-      ''')
-      create_test_file('sdl2_key.c', self.with_report_result(open(path_from_root('tests', 'sdl2_key.c')).read()))
+      }
 
-      self.compile_btest(['sdl2_key.c', '-o', 'page.html'] + defines + ['-s', 'USE_SDL=2', '--pre-js', 'pre.js', '-s', '''EXPORTED_FUNCTIONS=['_main', '_one']'''])
-      self.run_browser('page.html', '', '/report_result?37182145')
+      function keyup(c) {
+        var event = new KeyboardEvent("keyup", { 'keyCode': c, 'charCode': c, 'view': window, 'bubbles': true, 'cancelable': true });
+        document.dispatchEvent(event);
+      }
+    ''')
+    create_test_file('sdl2_key.c', self.with_report_result(open(path_from_root('tests', 'sdl2_key.c')).read()))
+
+    self.compile_btest(['sdl2_key.c', '-o', 'page.html', '-s', 'USE_SDL=2', '--pre-js', 'pre.js', '-s', '''EXPORTED_FUNCTIONS=['_main', '_one']'''])
+    self.run_browser('page.html', '', '/report_result?37182145')
 
   def test_sdl2_text(self):
     create_test_file('pre.js', '''
@@ -4629,13 +4626,10 @@ def test_access_file_after_heap_resize(self):
     self.compile_btest(['page.c', '-s', 'WASM=1', '-s', 'ALLOW_MEMORY_GROWTH=1', '--preload-file', 'test.txt', '-o', 'page.html'])
     self.run_browser('page.html', 'hello from file', '/report_result?15')
 
-    # with separate file packager invocation, letting us affect heap copying
-    # or lack thereof
-    for file_packager_args in [[], ['--no-heap-copy']]:
-      print(file_packager_args)
-      self.run_process([PYTHON, FILE_PACKAGER, 'data.js', '--preload', 'test.txt', '--js-output=' + 'data.js'] + file_packager_args)
-      self.compile_btest(['page.c', '-s', 'WASM=1', '-s', 'ALLOW_MEMORY_GROWTH=1', '--pre-js', 'data.js', '-o', 'page.html', '-s', 'FORCE_FILESYSTEM=1'])
-      self.run_browser('page.html', 'hello from file', '/report_result?15')
+    # with separate file packager invocation
+    self.run_process([PYTHON, FILE_PACKAGER, 'data.js', '--preload', 'test.txt', '--js-output=' + 'data.js'])
+    self.compile_btest(['page.c', '-s', 'WASM=1', '-s', 'ALLOW_MEMORY_GROWTH=1', '--pre-js', 'data.js', '-o', 'page.html', '-s', 'FORCE_FILESYSTEM=1'])
+    self.run_browser('page.html', 'hello from file', '/report_result?15')
 
   def test_unicode_html_shell(self):
     create_test_file('main.cpp', self.with_report_result(r'''

tests/test_core.py

@@ -5834,7 +5834,7 @@ def test_mmap(self):
     self.do_run_in_out_file_test('tests', 'core', 'test_mmap.c')
 
   def test_mmap_file(self):
-    for extra_args in [[], ['--no-heap-copy']]:
+    for extra_args in [[]]:
       self.emcc_args += ['--embed-file', 'data.dat'] + extra_args
       x = 'data from the file........'
       s = ''

tools/file_packager.py

@@ -22,7 +22,7 @@
 
 Usage:
 
-  file_packager.py TARGET [--preload A [B..]] [--embed C [D..]] [--exclude E [F..]]] [--js-output=OUTPUT.js] [--no-force] [--use-preload-cache] [--indexedDB-name=EM_PRELOAD_CACHE] [--no-heap-copy] [--separate-metadata] [--lz4] [--use-preload-plugins]
+  file_packager.py TARGET [--preload A [B..]] [--embed C [D..]] [--exclude E [F..]]] [--js-output=OUTPUT.js] [--no-force] [--use-preload-cache] [--indexedDB-name=EM_PRELOAD_CACHE] [--separate-metadata] [--lz4] [--use-preload-plugins]
 
   --preload  ,
   --embed    See emcc --help for more details on those options.
@@ -42,10 +42,6 @@
 
   --indexedDB-name Use specified IndexedDB database name (Default: 'EM_PRELOAD_CACHE')
 
-  --no-heap-copy If specified, the preloaded filesystem is not copied inside the Emscripten HEAP, but kept in a separate typed array outside it.
-                 The default, if this is not specified, is to embed the VFS inside the HEAP, so that mmap()ing files in it is a no-op.
-                 Passing this flag optimizes for fread() usage, omitting it optimizes for mmap() usage.
-
   --separate-metadata Stores package metadata separately. Only applicable when preloading and js-output file is specified.
 
   --lz4 Uses LZ4. This compresses the data using LZ4 when this utility is run, then the client decompresses chunks on the fly, avoiding storing
@@ -81,7 +77,7 @@
 import json
 
 if len(sys.argv) == 1:
-  print('''Usage: file_packager.py TARGET [--preload A [B..]] [--embed C [D..]] [--exclude E [F..]]] [--js-output=OUTPUT.js] [--no-force] [--use-preload-cache] [--indexedDB-name=EM_PRELOAD_CACHE] [--no-heap-copy] [--separate-metadata] [--lz4] [--use-preload-plugins]
+  print('''Usage: file_packager.py TARGET [--preload A [B..]] [--embed C [D..]] [--exclude E [F..]]] [--js-output=OUTPUT.js] [--no-force] [--use-preload-cache] [--indexedDB-name=EM_PRELOAD_CACHE] [--separate-metadata] [--lz4] [--use-preload-plugins]
 See the source for more details.''')
   sys.exit(0)
 
@@ -177,12 +173,6 @@ def main():
   # offline cache instead.
   use_preload_cache = False
   indexeddb_name = 'EM_PRELOAD_CACHE'
-  # If set to True, the blob received from XHR is moved to the Emscripten HEAP,
-  # optimizing for mmap() performance (if ALLOW_MEMORY_GROWTH=0).
-  # If set to False, the XHR blob is kept intact, and fread()s etc. are performed
-  # directly to that data. This optimizes for minimal memory usage and fread()
-  # performance.
-  heap_copy = True
   # If set to True, the package metadata is stored separately from js-output
   # file which makes js-output file immutable to the package content changes.
   # If set to False, the package metadata is stored inside the js-output file
@@ -209,7 +199,7 @@ def main():
       indexeddb_name = arg.split('=', 1)[1] if '=' in arg else None
       leading = ''
     elif arg == '--no-heap-copy':
-      heap_copy = False
+      print('ignoring legacy flag --no-heap-copy (that is the only mode supported now)')
       leading = ''
     elif arg == '--separate-metadata':
       separate_metadata = True
@@ -497,16 +487,7 @@ def was_seen(name):
   if has_preloaded:
     if not lz4:
       # Get the big archive and split it up
-      if heap_copy:
-        use_data = '''
-          // copy the entire loaded file into a spot in the heap. Files will refer to slices in that. They cannot be freed though
-          // (we may be allocating before malloc is ready, during startup).
-          var ptr = Module['getMemory'](byteArray.length);
-          Module['HEAPU8'].set(byteArray, ptr);
-          DataRequest.prototype.byteArray = Module['HEAPU8'].subarray(ptr, ptr+byteArray.length);
-    '''
-      else:
-        use_data = '''
+      use_data = '''
           // Reuse the bytearray from the XHR as the source for file reads.
           DataRequest.prototype.byteArray = byteArray;
     '''