Reduce number of calls to wasmTable.grow #23633

martenrichter · 2025-02-09T10:40:28Z

Before, getEmptyTableSlot increased the wasmTable by 1, increasing the overhead if many calls of addFunction exceeded the table length. Now, an exponential scaling strategy is implemented.

#17891

martenrichter · 2025-02-09T10:41:17Z

Note: I have tested on a compiled Emscripten module, so I am not sure if the formatting is correct. I hope the test will show it.

hoodmane · 2025-02-09T11:21:47Z

src/lib/libaddfunction.js

@@ -151,15 +151,17 @@ addToLibrary({
    }
    // Grow the table
    try {
+      var growBy = {{{ from64Expr('wasmTable.length') }}};
+      freeTableIndexes.push(...Array.from({ length: growBy }, (_, i) => {{{ from64Expr('wasmTable.length') }}} + i));


It would use less memory to record the last used entry separately. Also I'm not sure we want to grow the table exponentially, @sbc100 was suggesting linear or quadratic.

Okay, I have changed the behavior. Unfortunately, the last commit tests failed, and I did not get a clue what was wrong. (It must be something with the templating.) Maybe the new one is better.
Regarding linear or quadratic scaling, is the initial size of the table somewhere available? We need it to achieve linear or quadratic scaling. I also think it is important that the growth is in a similar order of magnitude as the current size.

hoodmane · 2025-02-09T13:59:22Z

I was imagining something roughly like:

  $freeTableIndexes: [],
  $usedTableLength: 0, // TODO: initialize this correctly

  // Weak map of functions in the table to their indexes, created on first use.
  $functionsInTableMap: undefined,

  $getEmptyTableSlot__deps: ['$freeTableIndexes', '$wasmTable', '$usedTableLength'],
  $getEmptyTableSlot: () => {
    // Reuse a free index if there is one, otherwise grow.
    if (freeTableIndexes.length) {
      return freeTableIndexes.pop();
    }
    if (usedTableLength < wasmTable.length) {
      return usedTableLength++;
    } 
    // Grow the table
    try {
      /** @suppress {checkTypes} */
      wasmTable.grow({{{ toIndexType('wasmTable.length') }}});
    } catch (err) {
      if (!(err instanceof RangeError)) {
        throw err;
      }
      throw 'Unable to grow wasm table. Set ALLOW_TABLE_GROWTH.';
    }
    return usedTableLength++;
  },

martenrichter · 2025-02-09T14:22:18Z

Ok, I try if this works. But I am not completely sure, what needs to be changed in libdylink.js .

hoodmane · 2025-02-09T14:46:05Z

I think it should be something like:

        var tableBase = metadata.tableSize ? usedTableLength : 0;
        if (handle) {
          {{{ makeSetValue('handle', C_STRUCTS.dso.mem_allocated, '1', 'i8') }}};
          {{{ makeSetValue('handle', C_STRUCTS.dso.mem_addr, 'memoryBase', '*') }}};
          {{{ makeSetValue('handle', C_STRUCTS.dso.mem_size, 'metadata.memorySize', 'i32') }}};
          {{{ makeSetValue('handle', C_STRUCTS.dso.table_addr, 'tableBase', '*') }}};
          {{{ makeSetValue('handle', C_STRUCTS.dso.table_size, 'metadata.tableSize', 'i32') }}};
        }
      } else {
        memoryBase = {{{ makeGetValue('handle', C_STRUCTS.dso.mem_addr, '*') }}};
        tableBase = {{{ makeGetValue('handle', C_STRUCTS.dso.table_addr, '*') }}};
      }

      var tableGrowthNeeded = tableBase + metadata.tableSize - {{{ from64Expr('wasmTable.length') }}};
      if (tableGrowthNeeded > 0) {
          wasmTable.grow({{{ toIndexType('tableGrowthNeeded') }}});
      }
      usedTableLength = tableBase + metadata.tableSize

hoodmane · 2025-02-09T14:47:19Z

And probably $dumpTable in libdylink.js should use usedTableLength instead of wasmTable.length.

martenrichter · 2025-02-09T14:48:50Z

I agree, I came up with similar changes. But I am not familiar with the code....

martenrichter · 2025-02-09T14:50:05Z

But if you use dylink, would it not generally interfere, if you have added functions in the meantime...? (Beside this change)

hoodmane · 2025-02-09T14:52:53Z

Dynamic linking currently works fine with addFunction. The library exports get placed in a block at the end of the wasmTable, ignoring any freelist entries.

hoodmane · 2025-02-09T14:57:06Z

I think usedTableLength should also be initialized in two places: line 911 of preamble.js if !RELOCATABLE:

#if '$wasmTable' in addedLibraryItems && !RELOCATABLE
    wasmTable = wasmExports['__indirect_function_table'];
#if ALLOW_TABLE_GROWTH
    usedTableLength = wasmTable.length;
#endif
    {{{ receivedSymbol('wasmTable') }}}

and line 2276 of libcore.js:

#if RELOCATABLE
  // In RELOCATABLE mode we create the table in JS.
  $wasmTable: `=new WebAssembly.Table({
  'initial': {{{ toIndexType(INITIAL_TABLE) }}},
#if !ALLOW_TABLE_GROWTH
  'maximum': {{{ toIndexType(INITIAL_TABLE) }}},
#endif
#if MEMORY64 == 1
  'address': 'i64',
   // TODO(sbc): remove this alias for 'address' once both firefox and
   // chrome roll out the spec change.
   // See https://github.com/WebAssembly/memory64/pull/92
  'index': 'i64',
#endif
  'element': 'anyfunc'
});
`,
#if ALLOW_TABLE_GROWTH
   $wasmTable_postset: `usedTableLength = wasmTable.length`,
#endif
#else

But I'm not really sure where to stick the $usedTableLength definition to make that work...

martenrichter · 2025-02-09T14:57:06Z

Ok, I was just wondering, as it contains the if clause with one branch being first load. I was wondering, if a wasm was loaded a second time with an increased number of functions. As it recycles the position in the table, it will overwrite the function pointers. If it would be running on the os and not in the sandbox, we would have a vulnerability.

hoodmane · 2025-02-09T14:58:39Z

Are you saying that you're concerned that addFunction isn't thread safe? It's probably not.

martenrichter · 2025-02-09T15:02:12Z

No, I am concerned with the following scenario
1.) You load a lib first with say x functions.
2.) Then you use addfunction with some functions
3.) You load the lib a second time but now it has x +1 functions, and the x+1 will overwrite the function added in 2.
But I am not sure if this would work, I was just wondering because of firstLoad .

hoodmane · 2025-02-09T15:04:15Z

dynamic linking + pthreads is experimental:
https://emscripten.org/docs/compiling/Dynamic-Linking.html#pthreads-support

I think the safe way to use it is to make sure all library loading and addFunction calls happen before you start any threads.

hoodmane · 2025-02-09T15:05:31Z

firstLoad is about threads. It's true when one thread has already loaded the library and it is being loaded into a second thread. If you load two different versions of the same library that will probably cause crashes.

martenrichter · 2025-02-09T15:07:19Z

Ok, I understand now. But should it check, that it is not having a different number of table entries and throw?

kg · 2025-02-09T15:07:26Z

With fast table growing it's quite possible to hit the limit for how big a function table can be if your function table already started big. I've heard of applications with tables containing 100k entries or more.

Given that, you probably want to handle the failure case and attempt one retry where you grow by a smaller amount. Otherwise if you have an application with a table that has 400k entries and the size limit is 500k, it'd never be able to grow, even though in practice there's plenty of room to grow.

martenrichter · 2025-02-09T15:12:24Z

Ok, good point, a factor of 10 down? Or better 100 ?

kg · 2025-02-09T15:22:37Z

Your measurements showed that 'grow by 100' and 'grow by 1000' were both big improvements over 'grow by 1'. I'd suggest just falling back to a flat 100 or 1000. 100 is more likely to work, but if an application is so close to the limit that 1000 won't work, it's probably close to broken anyway, so I could understand going with 1000 for better performance. I'd personally pick 100.

kg · 2025-02-09T15:25:08Z

src/lib/libaddfunction.js

    // Grow the table
    try {
+      var growBy = lastUnused;
+      freeTableIndexes.push(...Array.from({ length: growBy - 1 }, (_, i) => lastUnused + i + 1));


This is probably going to be really expensive for large tables. You might want to track free ranges instead of free indices.

This already gone, I have changed it to the suggestions by hoodmane. I will push soon, but I have to fix a failing test.

sbc100 · 2025-02-09T15:28:32Z

No, I am concerned with the following scenario 1.) You load a lib first with say x functions. 2.) Then you use addfunction with some functions 3.) You load the lib a second time but now it has x +1 functions, and the x+1 will overwrite the function added in 2. But I am not sure if this would work, I was just wondering because of firstLoad .

You can't load the same dynamic library twice. If you were to give them different names and load two version of the same library each version will get its own separate table and memory allocation.

martenrichter · 2025-02-09T15:31:11Z

Ok, I have added the changes and the test failing before now works. But I did not run the full suite, I am waiting for CI.

martenrichter · 2025-02-09T15:32:31Z

Your measurements showed that 'grow by 100' and 'grow by 1000' were both big improvements over 'grow by 1'. I'd suggest just falling back to a flat 100 or 1000. 100 is more likely to work, but if an application is so close to the limit that 1000 won't work, it's probably close to broken anyway, so I could understand going with 1000 for better performance. I'd personally pick 100.

I have now implemented growing by 1000 and then falling back to 10. But other variants are also possible.

sbc100 · 2025-02-09T15:36:00Z

Your measurements showed that 'grow by 100' and 'grow by 1000' were both big improvements over 'grow by 1'. I'd suggest just falling back to a flat 100 or 1000. 100 is more likely to work, but if an application is so close to the limit that 1000 won't work, it's probably close to broken anyway, so I could understand going with 1000 for better performance. I'd personally pick 100.

I have now implemented growing by 1000 and then falling back to 10. But other variants are also possible.

It seems to me that unless you fall all the way back to "grow by 1" you might not ever be able to use the final available slots.

I was going to suggest that instead of implementing falloff we could just query the max length .. but I don't think WebAssembly.Memory exposes that.

martenrichter · 2025-02-09T15:39:22Z

Your measurements showed that 'grow by 100' and 'grow by 1000' were both big improvements over 'grow by 1'. I'd suggest just falling back to a flat 100 or 1000. 100 is more likely to work, but if an application is so close to the limit that 1000 won't work, it's probably close to broken anyway, so I could understand going with 1000 for better performance. I'd personally pick 100.

I have now implemented growing by 1000 and then falling back to 10. But other variants are also possible.

It seems to me that unless you fall all the way back to "grow by 1" you might not ever be able to use the final available slots.

I was going to suggest that instead of implementing falloff we could just query the max length .. but I don't think WebAssembly.Memory exposes that.

Ok, then we go with 1 . And yes the spec does not have this function. I could look in the source code of Chromium if this helps, but if it is not in the specs it can change anytime.

src/preamble.js

src/lib/libdylink.js

Also add an extra assert that should catch table layout inconsistencies between threads. I noticed this could be improved while reviewing emscripten-core#23633.

Also add an extra assert that should catch table layout inconsistencies between threads. I noticed this could be improved while reviewing #23633.

sbc100 · 2025-02-10T18:06:12Z

src/lib/libdylink.js

@@ -668,6 +669,7 @@ var LibraryDylink = {
 #endif
        wasmTable.grow({{{ toIndexType('metadata.tableSize') }}});
      }
+      usedTableLength = tableBase + metadata.tableSize;


I think this line should be moved up inside the above if (metadata.tableSize) check, no?

I think this makes sense.

test/interop/test_add_function.cpp

src/lib/libaddfunction.js

src/lib/libcore.js

martenrichter · 2025-02-15T13:05:04Z

The new attempt uses a different structure for freeTableIndexes, which uses less memory and may also be used to easily reuse freed indices.

Before, getEmptyTableSlot increased the wasmTable by 1, increasing the overhead if many calls of addFunction exceeded the table length. Now, an exponential scaling strategy is implemented. Co-authored-by: Alon Zakai <alonzakai@gmail.com>

martenrichter mentioned this pull request Feb 9, 2025

XPython startup time suffers from frequent calls to wasm grow jupyterlite/xeus#185

Open

hoodmane reviewed Feb 9, 2025

View reviewed changes

kg reviewed Feb 9, 2025

View reviewed changes

sbc100 reviewed Feb 9, 2025

View reviewed changes

src/preamble.js Outdated Show resolved Hide resolved

sbc100 reviewed Feb 9, 2025

View reviewed changes

src/lib/libdylink.js Outdated Show resolved Hide resolved

sbc100 mentioned this pull request Feb 9, 2025

Simplify dynamic linking table allocation. NFC #23634

Merged

sbc100 added a commit that referenced this pull request Feb 10, 2025

Simplify dynamic linking table allocation. NFC (#23634)

f3b1194

Also add an extra assert that should catch table layout inconsistencies between threads. I noticed this could be improved while reviewing #23633.

sbc100 reviewed Feb 10, 2025

View reviewed changes

test/interop/test_add_function.cpp Outdated Show resolved Hide resolved

sbc100 reviewed Feb 10, 2025

View reviewed changes

src/lib/libaddfunction.js Outdated Show resolved Hide resolved

kripken reviewed Feb 10, 2025

View reviewed changes

src/lib/libaddfunction.js Outdated Show resolved Hide resolved

martenrichter force-pushed the patch-1 branch from dc33cc5 to d485896 Compare February 11, 2025 07:29

sbc100 reviewed Feb 11, 2025

View reviewed changes

src/lib/libcore.js Outdated Show resolved Hide resolved

hoodmane reviewed Feb 11, 2025

View reviewed changes

src/lib/libcore.js Outdated Show resolved Hide resolved

sbc100 mentioned this pull request Feb 14, 2025

What is wasmTable used for? #23673

Open

Reduce number of calls to wasmTable.grow

94ead84

Before, getEmptyTableSlot increased the wasmTable by 1, increasing the overhead if many calls of addFunction exceeded the table length. Now, an exponential scaling strategy is implemented. Co-authored-by: Alon Zakai <alonzakai@gmail.com>

martenrichter force-pushed the patch-1 branch from 84d8d40 to 94ead84 Compare February 15, 2025 13:30

martenrichter added 3 commits February 15, 2025 14:26

Fix removeFunction

7f2fbde

Fix failing frow wasmTable

7487f6a

Adapt test

f58a189

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce number of calls to wasmTable.grow #23633

Reduce number of calls to wasmTable.grow #23633

martenrichter commented Feb 9, 2025

martenrichter commented Feb 9, 2025

hoodmane Feb 9, 2025

martenrichter Feb 9, 2025

hoodmane commented Feb 9, 2025 •

edited

Loading

martenrichter commented Feb 9, 2025

hoodmane commented Feb 9, 2025 •

edited

Loading

hoodmane commented Feb 9, 2025

martenrichter commented Feb 9, 2025

martenrichter commented Feb 9, 2025

hoodmane commented Feb 9, 2025 •

edited

Loading

hoodmane commented Feb 9, 2025

martenrichter commented Feb 9, 2025

hoodmane commented Feb 9, 2025

martenrichter commented Feb 9, 2025

hoodmane commented Feb 9, 2025

hoodmane commented Feb 9, 2025

martenrichter commented Feb 9, 2025

kg commented Feb 9, 2025

martenrichter commented Feb 9, 2025

kg commented Feb 9, 2025

kg Feb 9, 2025

martenrichter Feb 9, 2025

sbc100 commented Feb 9, 2025

martenrichter commented Feb 9, 2025

martenrichter commented Feb 9, 2025

sbc100 commented Feb 9, 2025

martenrichter commented Feb 9, 2025

sbc100 Feb 10, 2025

martenrichter Feb 10, 2025

martenrichter Feb 10, 2025

martenrichter commented Feb 15, 2025

Reduce number of calls to wasmTable.grow #23633

Are you sure you want to change the base?

Reduce number of calls to wasmTable.grow #23633

Conversation

martenrichter commented Feb 9, 2025

martenrichter commented Feb 9, 2025

hoodmane Feb 9, 2025

Choose a reason for hiding this comment

martenrichter Feb 9, 2025

Choose a reason for hiding this comment

hoodmane commented Feb 9, 2025 • edited Loading

martenrichter commented Feb 9, 2025

hoodmane commented Feb 9, 2025 • edited Loading

hoodmane commented Feb 9, 2025

martenrichter commented Feb 9, 2025

martenrichter commented Feb 9, 2025

hoodmane commented Feb 9, 2025 • edited Loading

hoodmane commented Feb 9, 2025

martenrichter commented Feb 9, 2025

hoodmane commented Feb 9, 2025

martenrichter commented Feb 9, 2025

hoodmane commented Feb 9, 2025

hoodmane commented Feb 9, 2025

martenrichter commented Feb 9, 2025

kg commented Feb 9, 2025

martenrichter commented Feb 9, 2025

kg commented Feb 9, 2025

kg Feb 9, 2025

Choose a reason for hiding this comment

martenrichter Feb 9, 2025

Choose a reason for hiding this comment

sbc100 commented Feb 9, 2025

martenrichter commented Feb 9, 2025

martenrichter commented Feb 9, 2025

sbc100 commented Feb 9, 2025

martenrichter commented Feb 9, 2025

sbc100 Feb 10, 2025

Choose a reason for hiding this comment

martenrichter Feb 10, 2025

Choose a reason for hiding this comment

martenrichter Feb 10, 2025

Choose a reason for hiding this comment

martenrichter commented Feb 15, 2025

hoodmane commented Feb 9, 2025 •

edited

Loading

hoodmane commented Feb 9, 2025 •

edited

Loading

hoodmane commented Feb 9, 2025 •

edited

Loading