From a119f9cac322071ce99dc5e7eafa0ca0a3c7e694 Mon Sep 17 00:00:00 2001 From: Clemens Hammacher Date: Thu, 12 Jul 2018 13:41:34 +0200 Subject: [PATCH] [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}. R=mstarzinger@chromium.org Bug: chromium:862123 Change-Id: Iba71030f91949d7453740c884de1d8a4f921c618 Reviewed-on: https://chromium-review.googlesource.com/1131182 Commit-Queue: Clemens Hammacher Reviewed-by: Michael Starzinger Cr-Commit-Position: refs/heads/master@{#54404} --- src/asmjs/asm-parser.cc | 6 +++--- src/asmjs/asm-parser.h | 9 ++++++++- src/signature.h | 23 ++++++++++++--------- src/wasm/module-compiler.cc | 22 ++++++++------------ src/wasm/module-decoder.cc | 2 +- src/wasm/signature-map.cc | 22 ++------------------ src/wasm/signature-map.h | 16 +++++---------- src/wasm/value-type.h | 2 ++ src/wasm/wasm-debug.cc | 4 ++-- src/wasm/wasm-interpreter.cc | 4 ++-- src/wasm/wasm-module-builder.cc | 32 ++++++------------------------ src/wasm/wasm-module-builder.h | 9 +-------- src/wasm/wasm-objects.cc | 2 +- test/cctest/wasm/wasm-run-utils.cc | 2 +- test/cctest/wasm/wasm-run-utils.h | 2 +- 15 files changed, 57 insertions(+), 100 deletions(-) diff --git a/src/asmjs/asm-parser.cc b/src/asmjs/asm-parser.cc index 1fca56b0fc..fee309d9fb 100644 --- a/src/asmjs/asm-parser.cc +++ b/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); diff --git a/src/asmjs/asm-parser.h b/src/asmjs/asm-parser.h index bddb8c62e9..ac8a05a028 100644 --- a/src/asmjs/asm-parser.h +++ b/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 function_name; - WasmModuleBuilder::SignatureMap cache; + ZoneUnorderedMap cache; + + // Constructor. + FunctionImportInfo(Vector name, Zone* zone) + : function_name(name), cache(zone) {} }; struct VarInfo { diff --git a/src/signature.h b/src/signature.h index 1c84703147..60950a93bb 100644 --- a/src/signature.h +++ b/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 MachineSignature; +template +size_t hash_value(const Signature& 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 diff --git a/src/wasm/module-compiler.cc b/src/wasm/module-compiler.cc index 0338c7dcf7..632d9e101f 100644 --- a/src/wasm/module-compiler.cc +++ b/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 key(is_import, *func->sig); + Handle& cached = cache_[key]; + if (!cached.is_null()) return cached; Handle 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> code_cache; - } cache_[2]; + using CacheKey = std::pair; + std::unordered_map, base::hash> cache_; }; // A helper class to simplify instantiating a module from a module object. @@ -1512,7 +1506,7 @@ int InstanceBuilder::ProcessImports(Handle 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 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++; diff --git a/src/wasm/module-decoder.cc b/src/wasm/module-decoder.cc index 3de2d0b293..bae8e4baf8 100644 --- a/src/wasm/module-decoder.cc +++ b/src/wasm/module-decoder.cc @@ -428,7 +428,7 @@ class ModuleDecoderImpl : public Decoder { static_cast(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(); diff --git a/src/wasm/signature-map.cc b/src/wasm/signature-map.cc index b4054596f8..dca3b1ef89 100644 --- a/src/wasm/signature-map.cc +++ b/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(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 diff --git a/src/wasm/signature-map.h b/src/wasm/signature-map.h index 3fab0fde16..5ed66976d5 100644 --- a/src/wasm/signature-map.h +++ b/src/wasm/signature-map.h @@ -5,16 +5,14 @@ #ifndef V8_WASM_SIGNATURE_MAP_H_ #define V8_WASM_SIGNATURE_MAP_H_ -#include +#include +#include "src/signature.h" #include "src/wasm/value-type.h" namespace v8 { namespace internal { -template -class Signature; - namespace wasm { using FunctionSig = Signature; @@ -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 map_; + std::unordered_map> map_; }; } // namespace wasm diff --git a/src/wasm/value-type.h b/src/wasm/value-type.h index cf0360e932..8522b3a500 100644 --- a/src/wasm/value-type.h +++ b/src/wasm/value-type.h @@ -24,6 +24,8 @@ enum ValueType : uint8_t { kWasmVar, }; +inline size_t hash_value(ValueType type) { return static_cast(type); } + // TODO(clemensh): Compute memtype and size from ValueType once we have c++14 // constexpr support. #define FOREACH_LOAD_TYPE(V) \ diff --git a/src/wasm/wasm-debug.cc b/src/wasm/wasm-debug.cc index 069d5e8ed6..b1f57fa8f8 100644 --- a/src/wasm/wasm-debug.cc +++ b/src/wasm/wasm-debug.cc @@ -703,9 +703,9 @@ Handle WasmDebugInfo::GetCWasmEntry( } Handle 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(map->FindOrInsert(sig)); + index = static_cast(map->FindOrInsert(*sig)); if (index == entries->length()) { entries = isolate->factory()->CopyFixedArrayAndGrow( entries, entries->length(), TENURED); diff --git a/src/wasm/wasm-interpreter.cc b/src/wasm/wasm-interpreter.cc index 022159e157..581277cbab 100644 --- a/src/wasm/wasm-interpreter.cc +++ b/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. diff --git a/src/wasm/wasm-module-builder.cc b/src/wasm/wasm-module-builder.cc index a3ddc00ce4..15a4b0bbf1 100644 --- a/src/wasm/wasm-module-builder.cc +++ b/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(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(signatures_.size()); + signature_map_.emplace(*sig, index); + signatures_.push_back(sig); + return index; } uint32_t WasmModuleBuilder::AllocateIndirectFunctions(uint32_t count) { diff --git a/src/wasm/wasm-module-builder.h b/src/wasm/wasm-module-builder.h index 19ca123f0e..db70502886 100644 --- a/src/wasm/wasm-module-builder.h +++ b/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 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 data_segments_; ZoneVector indirect_functions_; ZoneVector globals_; - SignatureMap signature_map_; + ZoneUnorderedMap signature_map_; int start_function_index_; uint32_t min_memory_size_; uint32_t max_memory_size_; diff --git a/src/wasm/wasm-objects.cc b/src/wasm/wasm-objects.cc index c941477c77..45910ddfb5 100644 --- a/src/wasm/wasm-objects.cc +++ b/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); } diff --git a/test/cctest/wasm/wasm-run-utils.cc b/test/cctest/wasm/wasm-run-utils.cc index 36fcf47049..ee358d2efb 100644 --- a/test/cctest/wasm/wasm-run-utils.cc +++ b/test/cctest/wasm/wasm-run-utils.cc @@ -168,7 +168,7 @@ void TestingModuleBuilder::PopulateIndirectFunctionTable() { int table_size = static_cast(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); diff --git a/test/cctest/wasm/wasm-run-utils.h b/test/cctest/wasm/wasm-run-utils.h index c09659733f..ca1c922dd4 100644 --- a/test/cctest/wasm/wasm-run-utils.h +++ b/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);