[wasm] Refactor SignatureMap to use unordered_map

An unordered_map typically provides better performance. Instead of a
compare function, we now need a hash function and equality defined on
{Signature<T>}.

R=mstarzinger@chromium.org

Bug: chromium:862123
Change-Id: Iba71030f91949d7453740c884de1d8a4f921c618
Reviewed-on: https://chromium-review.googlesource.com/1131182
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54404}

src/asmjs/asm-parser.cc

@@ -553,7 +553,7 @@ void AsmJsParser::ValidateModuleVarImport(VarInfo* info,
     } else {
       info->kind = VarKind::kImportedFunction;
       info->import = new (zone()->New(sizeof(FunctionImportInfo)))
-          FunctionImportInfo({name, WasmModuleBuilder::SignatureMap(zone())});
+          FunctionImportInfo(name, zone());
       info->mutable_variable = false;
     }
   }
@@ -2210,14 +2210,14 @@ AsmType* AsmJsParser::ValidateCall() {
     DCHECK_NOT_NULL(function_info->import);
     // TODO(bradnelson): Factor out.
     uint32_t index;
-    auto it = function_info->import->cache.find(sig);
+    auto it = function_info->import->cache.find(*sig);
     if (it != function_info->import->cache.end()) {
       index = it->second;
       DCHECK(function_info->function_defined);
     } else {
       index =
           module_builder_->AddImport(function_info->import->function_name, sig);
-      function_info->import->cache[sig] = index;
+      function_info->import->cache[*sig] = index;
       function_info->function_defined = true;
     }
     current_function_builder_->AddAsmWasmOffset(call_pos, to_number_pos);

src/asmjs/asm-parser.h

@@ -76,9 +76,16 @@ class AsmJsParser {
   };
   // clang-format on
 
+  // A single import in asm.js can require multiple imports in wasm, if the
+  // function is used with different signatures. {cache} keeps the wasm
+  // imports for the single asm.js import of name {function_name}.
   struct FunctionImportInfo {
     Vector<const char> function_name;
-    WasmModuleBuilder::SignatureMap cache;
+    ZoneUnorderedMap<FunctionSig, uint32_t> cache;
+
+    // Constructor.
+    FunctionImportInfo(Vector<const char> name, Zone* zone)
+        : function_name(name), cache(zone) {}
   };
 
   struct VarInfo {

src/signature.h

@@ -5,6 +5,7 @@
 #ifndef V8_SIGNATURE_H_
 #define V8_SIGNATURE_H_
 
+#include "src/base/functional.h"
 #include "src/base/iterator.h"
 #include "src/machine-type.h"
 #include "src/zone/zone.h"
@@ -46,16 +47,13 @@ class Signature : public ZoneObject {
     return {reps_, reps_ + return_count_ + parameter_count_};
   }
 
-  bool Equals(const Signature* that) const {
-    if (this == that) return true;
-    if (this->parameter_count() != that->parameter_count()) return false;
-    if (this->return_count() != that->return_count()) return false;
-    size_t size = this->return_count() + this->parameter_count();
-    for (size_t i = 0; i < size; i++) {
-      if (this->reps_[i] != that->reps_[i]) return false;
-    }
-    return true;
+  bool operator==(const Signature& other) const {
+    if (this == &other) return true;
+    if (parameter_count() != other.parameter_count()) return false;
+    if (return_count() != other.return_count()) return false;
+    return std::equal(all().begin(), all().end(), other.all().begin());
   }
+  bool operator!=(const Signature& other) const { return !(*this == other); }
 
   // For incrementally building signatures.
   class Builder {
@@ -101,6 +99,13 @@ class Signature : public ZoneObject {
 
 typedef Signature<MachineType> MachineSignature;
 
+template <typename T>
+size_t hash_value(const Signature<T>& sig) {
+  size_t hash = base::hash_combine(sig.parameter_count(), sig.return_count());
+  for (const T& t : sig.all()) hash = base::hash_combine(hash, t);
+  return hash;
+}
+
 }  // namespace internal
 }  // namespace v8
 

src/wasm/module-compiler.cc

@@ -179,29 +179,23 @@ class JSToWasmWrapperCache {
     const wasm::WasmModule* module = native_module->module();
     const wasm::WasmFunction* func = &module->functions[func_index];
     bool is_import = func_index < module->num_imported_functions;
-    auto& cache = cache_[is_import ? 1 : 0];
-    int cached_idx = cache.sig_map.Find(func->sig);
-    if (cached_idx >= 0) return cache.code_cache[cached_idx];
+    std::pair<bool, wasm::FunctionSig> key(is_import, *func->sig);
+    Handle<Code>& cached = cache_[key];
+    if (!cached.is_null()) return cached;
 
     Handle<Code> code =
         compiler::CompileJSToWasmWrapper(isolate, native_module, func->sig,
                                          is_import, use_trap_handler)
             .ToHandleChecked();
-    uint32_t new_cache_idx = cache.sig_map.FindOrInsert(func->sig);
-    DCHECK_EQ(cache.code_cache.size(), new_cache_idx);
-    USE(new_cache_idx);
-    cache.code_cache.push_back(code);
+    cached = code;
     return code;
   }
 
  private:
   // We generate different code for calling imports than calling wasm functions
   // in this module. Both are cached separately.
-  // [0] for non-imports, [1] for imports.
-  struct Cache {
-    wasm::SignatureMap sig_map;
-    std::vector<Handle<Code>> code_cache;
-  } cache_[2];
+  using CacheKey = std::pair<bool, wasm::FunctionSig>;
+  std::unordered_map<CacheKey, Handle<Code>, base::hash<CacheKey>> cache_;
 };
 
 // A helper class to simplify instantiating a module from a module object.
@@ -1512,7 +1506,7 @@ int InstanceBuilder::ProcessImports(Handle<WasmInstanceObject> instance) {
               imported_instance->module()
                   ->functions[imported_function->function_index()]
                   .sig;
-          if (!imported_sig->Equals(expected_sig)) {
+          if (*imported_sig != *expected_sig) {
             ReportLinkError(
                 "imported function does not match the expected type", index,
                 module_name, import_name);
@@ -1611,7 +1605,7 @@ int InstanceBuilder::ProcessImports(Handle<WasmInstanceObject> instance) {
                                  ->functions[target->function_index()]
                                  .sig;
           IndirectFunctionTableEntry(instance, i)
-              .set(module_->signature_map.Find(sig), *imported_instance,
+              .set(module_->signature_map.Find(*sig), *imported_instance,
                    exported_call_target);
         }
         num_imported_tables++;

src/wasm/module-decoder.cc

@@ -428,7 +428,7 @@ class ModuleDecoderImpl : public Decoder {
             static_cast<int>(pc_ - start_));
       FunctionSig* s = consume_sig(module_->signature_zone.get());
       module_->signatures.push_back(s);
-      uint32_t id = s ? module_->signature_map.FindOrInsert(s) : 0;
+      uint32_t id = s ? module_->signature_map.FindOrInsert(*s) : 0;
       module_->signature_ids.push_back(id);
     }
     module_->signature_map.Freeze();

src/wasm/signature-map.cc

@@ -10,7 +10,7 @@ namespace v8 {
 namespace internal {
 namespace wasm {
 
-uint32_t SignatureMap::FindOrInsert(FunctionSig* sig) {
+uint32_t SignatureMap::FindOrInsert(const FunctionSig& sig) {
   CHECK(!frozen_);
   auto pos = map_.find(sig);
   if (pos != map_.end()) {
@@ -22,7 +22,7 @@ uint32_t SignatureMap::FindOrInsert(FunctionSig* sig) {
   }
 }
 
-int32_t SignatureMap::Find(FunctionSig* sig) const {
+int32_t SignatureMap::Find(const FunctionSig& sig) const {
   auto pos = map_.find(sig);
   if (pos != map_.end()) {
     return static_cast<int32_t>(pos->second);
@@ -31,24 +31,6 @@ int32_t SignatureMap::Find(FunctionSig* sig) const {
   }
 }
 
-bool SignatureMap::CompareFunctionSigs::operator()(FunctionSig* a,
-                                                   FunctionSig* b) const {
-  if (a == b) return false;
-  if (a->return_count() < b->return_count()) return true;
-  if (a->return_count() > b->return_count()) return false;
-  if (a->parameter_count() < b->parameter_count()) return true;
-  if (a->parameter_count() > b->parameter_count()) return false;
-  for (size_t r = 0; r < a->return_count(); r++) {
-    if (a->GetReturn(r) < b->GetReturn(r)) return true;
-    if (a->GetReturn(r) > b->GetReturn(r)) return false;
-  }
-  for (size_t p = 0; p < a->parameter_count(); p++) {
-    if (a->GetParam(p) < b->GetParam(p)) return true;
-    if (a->GetParam(p) > b->GetParam(p)) return false;
-  }
-  return false;
-}
-
 }  // namespace wasm
 }  // namespace internal
 }  // namespace v8

src/wasm/signature-map.h

@@ -5,16 +5,14 @@
 #ifndef V8_WASM_SIGNATURE_MAP_H_
 #define V8_WASM_SIGNATURE_MAP_H_
 
-#include <map>
+#include <unordered_map>
 
+#include "src/signature.h"
 #include "src/wasm/value-type.h"
 
 namespace v8 {
 namespace internal {
 
-template <typename T>
-class Signature;
-
 namespace wasm {
 
 using FunctionSig = Signature<ValueType>;
@@ -30,22 +28,18 @@ class V8_EXPORT_PRIVATE SignatureMap {
   MOVE_ONLY_WITH_DEFAULT_CONSTRUCTORS(SignatureMap);
 
   // Gets the index for a signature, assigning a new index if necessary.
-  uint32_t FindOrInsert(FunctionSig* sig);
+  uint32_t FindOrInsert(const FunctionSig& sig);
 
   // Gets the index for a signature, returning {-1} if not found.
-  int32_t Find(FunctionSig* sig) const;
+  int32_t Find(const FunctionSig& sig) const;
 
   // Disallows further insertions to this signature map.
   void Freeze() { frozen_ = true; }
 
  private:
-  // TODO(wasm): use a hashmap instead of an ordered map?
-  struct CompareFunctionSigs {
-    bool operator()(FunctionSig* a, FunctionSig* b) const;
-  };
   uint32_t next_ = 0;
   bool frozen_ = false;
-  std::map<FunctionSig*, uint32_t, CompareFunctionSigs> map_;
+  std::unordered_map<FunctionSig, uint32_t, base::hash<FunctionSig>> map_;
 };
 
 }  // namespace wasm

src/wasm/value-type.h

@@ -24,6 +24,8 @@ enum ValueType : uint8_t {
   kWasmVar,
 };
 
+inline size_t hash_value(ValueType type) { return static_cast<size_t>(type); }
+
 // TODO(clemensh): Compute memtype and size from ValueType once we have c++14
 // constexpr support.
 #define FOREACH_LOAD_TYPE(V) \

src/wasm/wasm-debug.cc

@@ -703,9 +703,9 @@ Handle<JSFunction> WasmDebugInfo::GetCWasmEntry(
   }
   Handle<FixedArray> entries(debug_info->c_wasm_entries(), isolate);
   wasm::SignatureMap* map = debug_info->c_wasm_entry_map()->raw();
-  int32_t index = map->Find(sig);
+  int32_t index = map->Find(*sig);
   if (index == -1) {
-    index = static_cast<int32_t>(map->FindOrInsert(sig));
+    index = static_cast<int32_t>(map->FindOrInsert(*sig));
     if (index == entries->length()) {
       entries = isolate->factory()->CopyFixedArrayAndGrow(
           entries, entries->length(), TENURED);

src/wasm/wasm-interpreter.cc

@@ -2846,7 +2846,7 @@ class ThreadImpl {
             module()->signature_ids[code->function->sig_index];
         int expected_canonical_id = module()->signature_ids[sig_index];
         DCHECK_EQ(function_canonical_id,
-                  module()->signature_map.Find(code->function->sig));
+                  module()->signature_map.Find(*code->function->sig));
         if (function_canonical_id != expected_canonical_id) {
           return {ExternalCallResult::SIGNATURE_MISMATCH};
         }
@@ -2857,7 +2857,7 @@ class ThreadImpl {
     Isolate* isolate = instance_object_->GetIsolate();
     uint32_t expected_sig_id = module()->signature_ids[sig_index];
     DCHECK_EQ(expected_sig_id,
-              module()->signature_map.Find(module()->signatures[sig_index]));
+              module()->signature_map.Find(*module()->signatures[sig_index]));
 
     // The function table is stored in the instance.
     // TODO(wasm): the wasm interpreter currently supports only one table.

src/wasm/wasm-module-builder.cc

@@ -249,33 +249,13 @@ void WasmModuleBuilder::AddDataSegment(const byte* data, uint32_t size,
   }
 }
 
-bool WasmModuleBuilder::CompareFunctionSigs::operator()(FunctionSig* a,
-                                                        FunctionSig* b) const {
-  if (a->return_count() < b->return_count()) return true;
-  if (a->return_count() > b->return_count()) return false;
-  if (a->parameter_count() < b->parameter_count()) return true;
-  if (a->parameter_count() > b->parameter_count()) return false;
-  for (size_t r = 0; r < a->return_count(); r++) {
-    if (a->GetReturn(r) < b->GetReturn(r)) return true;
-    if (a->GetReturn(r) > b->GetReturn(r)) return false;
-  }
-  for (size_t p = 0; p < a->parameter_count(); p++) {
-    if (a->GetParam(p) < b->GetParam(p)) return true;
-    if (a->GetParam(p) > b->GetParam(p)) return false;
-  }
-  return false;
-}
-
 uint32_t WasmModuleBuilder::AddSignature(FunctionSig* sig) {
-  SignatureMap::iterator pos = signature_map_.find(sig);
-  if (pos != signature_map_.end()) {
-    return pos->second;
-  } else {
-    uint32_t index = static_cast<uint32_t>(signatures_.size());
-    signature_map_[sig] = index;
-    signatures_.push_back(sig);
-    return index;
-  }
+  auto sig_entry = signature_map_.find(*sig);
+  if (sig_entry != signature_map_.end()) return sig_entry->second;
+  uint32_t index = static_cast<uint32_t>(signatures_.size());
+  signature_map_.emplace(*sig, index);
+  signatures_.push_back(sig);
+  return index;
 }
 
 uint32_t WasmModuleBuilder::AllocateIndirectFunctions(uint32_t count) {

src/wasm/wasm-module-builder.h

@@ -241,13 +241,6 @@ class V8_EXPORT_PRIVATE WasmModuleBuilder : public ZoneObject {
   void WriteTo(ZoneBuffer& buffer) const;
   void WriteAsmJsOffsetTable(ZoneBuffer& buffer) const;
 
-  // TODO(titzer): use SignatureMap from signature-map.h here.
-  // This signature map is zone-allocated, but the other is heap allocated.
-  struct CompareFunctionSigs {
-    bool operator()(FunctionSig* a, FunctionSig* b) const;
-  };
-  typedef ZoneMap<FunctionSig*, uint32_t, CompareFunctionSigs> SignatureMap;
-
   Zone* zone() { return zone_; }
 
   FunctionSig* GetSignature(uint32_t index) { return signatures_[index]; }
@@ -290,7 +283,7 @@ class V8_EXPORT_PRIVATE WasmModuleBuilder : public ZoneObject {
   ZoneVector<WasmDataSegment> data_segments_;
   ZoneVector<uint32_t> indirect_functions_;
   ZoneVector<WasmGlobal> globals_;
-  SignatureMap signature_map_;
+  ZoneUnorderedMap<FunctionSig, uint32_t> signature_map_;
   int start_function_index_;
   uint32_t min_memory_size_;
   uint32_t max_memory_size_;

src/wasm/wasm-objects.cc

@@ -845,7 +845,7 @@ void WasmTableObject::UpdateDispatchTables(
         isolate);
     // Note that {SignatureMap::Find} may return {-1} if the signature is
     // not found; it will simply never match any check.
-    auto sig_id = to_instance->module()->signature_map.Find(sig);
+    auto sig_id = to_instance->module()->signature_map.Find(*sig);
     IndirectFunctionTableEntry(to_instance, table_index)
         .set(sig_id, *from_instance, call_target);
   }

test/cctest/wasm/wasm-run-utils.cc

@@ -168,7 +168,7 @@ void TestingModuleBuilder::PopulateIndirectFunctionTable() {
     int table_size = static_cast<int>(instance->indirect_function_table_size());
     for (int j = 0; j < table_size; j++) {
       WasmFunction& function = test_module_->functions[table.values[j]];
-      int sig_id = test_module_->signature_map.Find(function.sig);
+      int sig_id = test_module_->signature_map.Find(*function.sig);
       auto target =
           native_module_->GetCallTargetForFunction(function.func_index);
       IndirectFunctionTableEntry(instance, j).set(sig_id, *instance, target);

test/cctest/wasm/wasm-run-utils.h

@@ -116,7 +116,7 @@ class TestingModuleBuilder {
     DCHECK_EQ(test_module_->signatures.size(),
               test_module_->signature_ids.size());
     test_module_->signatures.push_back(sig);
-    auto canonical_sig_num = test_module_->signature_map.FindOrInsert(sig);
+    auto canonical_sig_num = test_module_->signature_map.FindOrInsert(*sig);
     test_module_->signature_ids.push_back(canonical_sig_num);
     size_t size = test_module_->signatures.size();
     CHECK_GT(127, size);