[wasm] Add support for multiple indirect function tables

src/wasm/wasm-module.cc

Index: src/wasm/wasm-module.cc
diff --git a/src/wasm/wasm-module.cc b/src/wasm/wasm-module.cc
index 258950a91bba98e2ce0e7eb58c0d703131f6fa42..ef1e4c3a6844333c87d39ebc0a841d439676df01 100644
--- a/src/wasm/wasm-module.cc
+++ b/src/wasm/wasm-module.cc
@@ -152,11 +152,11 @@ enum CompiledWasmObjectFields {
   kExports,          // maybe FixedArray of FixedArray of WasmExportMetadata
                      // structure
   kStartupFunction,  // maybe FixedArray of WasmExportMetadata structure
-  kIndirectFunctionTableSize,       // Smi. Size of indirect function table.
-  kIndirectFunctionTablePrototype,  // maybe FixedArray
-  kModuleBytes,                     // maybe String
-  kFunctionNameTable,               // maybe ByteArray
-  kMinRequiredMemory,               // Smi. an uint32_t
+  kIndirectFunctionTable,  // maybe FixedArray of FixedArray of
+                           // WasmTableMetadata
+  kModuleBytes,            // maybe String
+  kFunctionNameTable,      // maybe ByteArray
+  kMinRequiredMemory,      // Smi. an uint32_t
   // The following 2 are either together present or absent:
   kDataSegmentsInfo,  // maybe FixedArray of FixedArray respecting the
                       // WasmSegmentInfo structure
@@ -190,6 +190,12 @@ enum WasmSegmentInfo {
   kWasmSegmentInfoSize  // Sentinel value.
 };
 
+enum WasmTableMetadata {
+  kSize,                  // Smi. an uint32_t
+  kTable,                 // FixedArray of indirect function table
+  kWasmTableMetadataSize  // Sentinel value.
+};
+
 uint32_t GetMinModuleMemSize(const WasmModule* module) {
   return WasmModule::kPageSize * module->min_mem_pages;
 }
@@ -255,39 +261,6 @@ void SaveDataSegmentInfo(Factory* factory, const WasmModule* module,
   compiled_module->set(kDataSegments, *data);
 }
 
-MaybeHandle<FixedArray> BuildFunctionTable(Isolate* isolate,
-                                           const WasmModule* module) {
-  // Compute the size of the indirect function table
-  uint32_t table_size = module->FunctionTableSize();
-  if (table_size == 0) {
-    return MaybeHandle<FixedArray>();
-  }
-
-  DCHECK_GE(table_size, module->function_table.size());
-
-  Handle<FixedArray> fixed = isolate->factory()->NewFixedArray(2 * table_size);
-  for (uint32_t i = 0; i < module->function_table.size(); ++i) {
-    const WasmFunction* function =
-        &module->functions[module->function_table[i]];
-    int fixed_array_index = static_cast<int>(i);
-    fixed->set(fixed_array_index, Smi::FromInt(function->sig_index));
-    fixed->set(fixed_array_index + table_size,
-               Smi::FromInt(module->function_table[fixed_array_index]));
-  }
-
-  // Set the remaining elements to -1 (instead of "undefined"). These
-  // elements are accessed directly as SMIs (without a check). On 64-bit
-  // platforms, it is possible to have the top bits of "undefined" take
-  // small integer values (or zero), which are more likely to be equal to
-  // the signature index we check against.
-  for (uint32_t i = static_cast<uint32_t>(module->function_table.size());
-       i < table_size;
-       ++i) {
-    fixed->set(i, Smi::FromInt(-1));
-  }
-  return fixed;
-}
-
 void PatchFunctionTable(Handle<Code> code,
                         Handle<FixedArray> old_indirect_table,
                         Handle<FixedArray> new_indirect_table) {
@@ -459,7 +432,6 @@ WasmModule::WasmModule(byte* module_start)
       start_function_index(-1),
       origin(kWasmOrigin),
       globals_size(0),
-      indirect_table_size(0),
       pending_tasks(new base::Semaphore(0)) {}
 
 static MaybeHandle<JSFunction> ReportFFIError(
@@ -1025,10 +997,19 @@ MaybeHandle<FixedArray> WasmModule::CompileFunctions(
   temp_instance_for_compilation.mem_start = nullptr;
   temp_instance_for_compilation.globals_start = nullptr;
 
-  MaybeHandle<FixedArray> indirect_table = BuildFunctionTable(isolate, this);
-  if (!indirect_table.is_null()) {
-    temp_instance_for_compilation.function_table =
-        indirect_table.ToHandleChecked();
+  MaybeHandle<FixedArray> indirect_table =
+      function_table.size()
+          ? factory->NewFixedArray(static_cast<int>(function_table.size()))
+          : MaybeHandle<FixedArray>();
+  for (uint32_t i = 0; i < function_table.size(); i++) {
+    Handle<FixedArray> values = wasm::BuildFunctionTable(isolate, i, this);
+    temp_instance_for_compilation.function_table[i] = values;

[ahaas, 2016/07/25 19:23:44]

Why do you store the function table in the temp_instance_for_compilation? Why
does temp_instance_for_compilation exist in the first place?

[ddchen, 2016/07/25 22:17:36]

I'm not entirely familiar with this code, but I believe that modules are first
compiled (sequentially or in parallel), and are then separately instantiated.
The reason that they are stored is that the tables need to be accessible from
wasm-compiler.cc; if a table is used by an indirect call, then it is cached as a
heap constant and referenced by the graph builder.

+
+    Handle<FixedArray> metadata =
+        isolate->factory()->NewFixedArray(kWasmTableMetadataSize);
+    metadata->set(kSize, Smi::FromInt(function_table[i].size));
+    metadata->set(kTable, *values);
+    indirect_table.ToHandleChecked()->set(i, *metadata);
   }
 
   HistogramTimerScope wasm_compile_module_time_scope(
@@ -1075,11 +1056,8 @@ MaybeHandle<FixedArray> WasmModule::CompileFunctions(
   Handle<FixedArray> ret =
       factory->NewFixedArray(kCompiledWasmObjectTableSize, TENURED);
   ret->set(kFunctions, *compiled_functions);
-  ret->set(kIndirectFunctionTableSize,
-           Smi::FromInt(static_cast<int>(function_table.size())));
   if (!indirect_table.is_null()) {
-    ret->set(kIndirectFunctionTablePrototype,
-             *indirect_table.ToHandleChecked());
+    ret->set(kIndirectFunctionTable, *indirect_table.ToHandleChecked());
   }
   Handle<FixedArray> import_data = GetImportsMetadata(factory, this);
   ret->set(kImportData, *import_data);
@@ -1186,23 +1164,33 @@ Handle<FixedArray> CloneModuleForInstance(Isolate* isolate,
   Handle<FixedArray> clone_wasm_functions =
       factory->CopyFixedArray(orig_wasm_functions);
   clone->set(kFunctions, *clone_wasm_functions);
-
-  MaybeHandle<FixedArray> maybe_indirect_table =
-      original->GetValue<FixedArray>(kIndirectFunctionTablePrototype);
-
-  Handle<FixedArray> indirect_table;
-  Handle<FixedArray> old_table;
-  if (maybe_indirect_table.ToHandle(&old_table)) {
-    indirect_table = factory->CopyFixedArray(old_table);
-    clone->set(kIndirectFunctionTablePrototype, *indirect_table);
-  }
-
   for (int i = 0; i < orig_wasm_functions->length(); ++i) {
     Handle<Code> orig_code = orig_wasm_functions->GetValueChecked<Code>(i);
     Handle<Code> cloned_code = factory->CopyCode(orig_code);
     clone_wasm_functions->set(i, *cloned_code);
-    if (!maybe_indirect_table.is_null()) {
-      PatchFunctionTable(cloned_code, old_table, indirect_table);
+  }
+
+  MaybeHandle<FixedArray> maybe_indirect_table =
+      original->GetValue<FixedArray>(kIndirectFunctionTable);
+  Handle<FixedArray> indirect_table;
+  if (maybe_indirect_table.ToHandle(&indirect_table)) {
+    Handle<FixedArray> clone_table = factory->CopyFixedArray(indirect_table);
+    clone->set(kIndirectFunctionTable, *clone_table);
+    for (int i = 0; i < clone_table->length(); ++i) {
+      Handle<FixedArray> orig_metadata =
+          clone_table->GetValueChecked<FixedArray>(i);
+      Handle<FixedArray> clone_metadata =
+          factory->CopyFixedArray(orig_metadata);
+      clone_table->set(i, *clone_metadata);
+      Handle<FixedArray> orig_table =
+          clone_metadata->GetValueChecked<FixedArray>(kTable);
+      Handle<FixedArray> clone_table = factory->CopyFixedArray(orig_table);

[ahaas, 2016/07/25 19:23:45]

clone_table is already used above, could you use a different variable name here?

[ddchen, 2016/07/25 22:17:36]

Done.

+      clone_metadata->set(kTable, *clone_table);
+      // Update all the functions in each table
+      for (int i = 0; i < clone_wasm_functions->length(); ++i) {
+        Handle<Code> code = clone_wasm_functions->GetValueChecked<Code>(i);
+        PatchFunctionTable(code, orig_table, clone_table);
+      }
     }
   }
 
@@ -1274,14 +1262,12 @@ MaybeHandle<JSObject> WasmModule::Instantiate(
   Handle<FixedArray> code_table =
       compiled_module->GetValueChecked<FixedArray>(kFunctions);
 
-  {
-    std::vector<Handle<Code>> functions(
-        static_cast<size_t>(code_table->length()));
-    for (int i = 0; i < code_table->length(); ++i) {
-      functions[static_cast<size_t>(i)] = code_table->GetValueChecked<Code>(i);
-    }
-    LinkModuleFunctions(isolate, functions);
+  std::vector<Handle<Code>> functions(

[ahaas, 2016/07/25 19:23:45]

Why did you make this change?

[ddchen, 2016/07/25 22:17:36]

The reason I moved the {BuildFunctionTable} and {PopulateFunctionTable}
functions was to expose them for usage in for wasm-run-utils.h. Previously, the
implementations were copied into that file. But for {BuildFunctionTable}, one
implementation used the {std::vector} representation, while the other used the
{FixedArray} implementation. I standardized on the {std::vector} representation,
so I exposed it here.

+      static_cast<size_t>(code_table->length()));
+  for (int i = 0; i < code_table->length(); ++i) {
+    functions[static_cast<size_t>(i)] = code_table->GetValueChecked<Code>(i);
   }
+  LinkModuleFunctions(isolate, functions);
 
   RecordStats(isolate, code_table);
 
@@ -1306,17 +1292,15 @@ MaybeHandle<JSObject> WasmModule::Instantiate(
   FlushAssemblyCache(isolate, code_table);
 
   MaybeHandle<FixedArray> maybe_indirect_table =
-      compiled_module->GetValue<FixedArray>(kIndirectFunctionTablePrototype);
+      compiled_module->GetValue<FixedArray>(kIndirectFunctionTable);
   Handle<FixedArray> indirect_table;
   if (maybe_indirect_table.ToHandle(&indirect_table)) {
-    int table_size =
-        Smi::cast(compiled_module->get(kIndirectFunctionTableSize))->value();
-    int half_point = indirect_table->length() / 2;
-    for (int i = half_point; i < half_point + table_size; ++i) {
-      int index = Smi::cast(indirect_table->get(i))->value();
-      DCHECK_GE(index, 0);
-      DCHECK_LT(index, code_table->length());
-      indirect_table->set(i, code_table->get(index));
+    for (int i = 0; i < indirect_table->length(); ++i) {
+      Handle<FixedArray> metadata =
+          indirect_table->GetValueChecked<FixedArray>(i);
+      uint32_t size = Smi::cast(metadata->get(kSize))->value();
+      Handle<FixedArray> table = metadata->GetValueChecked<FixedArray>(kTable);
+      wasm::PopulateFunctionTable(table, size, &functions);
     }
     js_object->SetInternalField(kWasmModuleFunctionTable, *indirect_table);
   }
@@ -1473,6 +1457,40 @@ bool UpdateWasmModuleMemory(Handle<JSObject> object, Address old_start,
   return true;
 }
 
+Handle<FixedArray> BuildFunctionTable(Isolate* isolate, uint32_t index,

[ahaas, 2016/07/25 19:23:45]

Why did you move this function?

[ddchen, 2016/07/25 22:17:36]

See above.

+                                      const WasmModule* module) {
+  const WasmTable* table = &module->function_table[index];
+  DCHECK_EQ(table->size, table->values.size());
+  DCHECK_GE(table->max_size, table->size);
+  Handle<FixedArray> values =
+      isolate->factory()->NewFixedArray(2 * table->max_size);
+  for (uint32_t i = 0; i < table->size; ++i) {
+    const WasmFunction* function = &module->functions[table->values[i]];
+    values->set(i, Smi::FromInt(function->sig_index));
+    values->set(i + table->max_size, Smi::FromInt(table->values[i]));
+  }
+  // Set the remaining elements to -1 (instead of "undefined"). These
+  // elements are accessed directly as SMIs (without a check). On 64-bit
+  // platforms, it is possible to have the top bits of "undefined" take
+  // small integer values (or zero), which are more likely to be equal to
+  // the signature index we check against.
+  for (uint32_t i = table->size; i < table->max_size; i++) {
+    values->set(i, Smi::FromInt(-1));
+  }
+  return values;
+}
+
+void PopulateFunctionTable(Handle<FixedArray> table, uint32_t table_size,
+                           const std::vector<Handle<Code>>* code_table) {
+  int max_size = table->length() / 2;
+  for (int i = max_size; i < max_size + table_size; ++i) {
+    int index = Smi::cast(table->get(i))->value();
+    DCHECK_GE(index, 0);
+    DCHECK_LT(index, code_table->size());
+    table->set(i, *(*code_table)[index]);
+  }
+}
+
 int GetNumberOfFunctions(JSObject* wasm) {
   Object* func_names_obj = wasm->GetInternalField(kWasmFunctionNamesArray);
   // TODO(clemensh): this looks inside an array constructed elsewhere. Refactor.