Add a new HashMap implementation #60881
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
reduz
force-pushed
the
new-hash-map
branch
2 times, most recently
from
1a4a182 to
6a7059d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
timothyqiu
reviewed
May 8, 2022
This comment was marked as outdated.
This comment was marked as outdated.
timothyqiu
reviewed
May 8, 2022
|
@bruvzg for Variant I think you have to explicitly tell it to use the VariantHasher hash and VariantComparator. dictionary.cpp uses it like: |
|
btw since its not yet ready to merge (needs more testing, reviews and discussions) I am marking as draft. |
This seems to work. With the all off suggestions above applied, it seems to be building and running fine - #60885 |
reduz
force-pushed
the
new-hash-map
branch
2 times, most recently
from
e895866 to
7b62de6
Compare
bruvzg
reviewed
May 8, 2022
reduz
force-pushed
the
new-hash-map
branch
2 times, most recently
from
d1ed3e3 to
40b1db4
Compare
bruvzg
reviewed
May 8, 2022
bruvzg
reviewed
May 8, 2022
If you believe it is feasible to safely rename it everywhere in the code base at some point in time without breaking too much outstanding PRs and code in progress, why not rename the legacy class instead to discourage its use? Intention: move the disruption earlier in the development cycle and reduce the number of places you need to rename it (before you port use of other containers not called "HashMap".) |
|
This is super tight code. I'm not a reviewer, since I am new here in your code base :) and in any case I only found one suggestion: You could consider initializing "pos" to ~0 instead of 0 before calling _lookup_pos(..). Just in case someone in future messes up the code and uses "pos" in the case of _lookup_pos() == false. I realize you don't want to write to pos in the negative case for optimization purposes, but maybe initialize it to something that will force a crash instead of something that is always a valid bucket? No big deal, just an idea I wanted to share after spending time reading the code. |
|
Pushed another update to fix and re-enable the Mono build. diff --git a/.github/workflows/linux_builds.yml b/.github/workflows/linux_builds.yml
index b3af4db5d9..338278f461 100644
--- a/.github/workflows/linux_builds.yml
+++ b/.github/workflows/linux_builds.yml
@@ -19,16 +19,16 @@ jobs:
fail-fast: false
matrix:
include:
-# - name: Editor w/ Mono (target=release_debug, tools=yes, tests=yes)
-# cache-name: linux-editor-mono
-# target: release_debug
-# tools: true
-# tests: false # Disabled due freeze caused by mix Mono build and CI
-# sconsflags: module_mono_enabled=yes mono_static=yes mono_glue=no
-# doc-test: true
-# bin: "./bin/godot.linuxbsd.opt.tools.64.mono"
-# build-mono: true
-# artifact: true
+ - name: Editor w/ Mono (target=release_debug, tools=yes, tests=yes)
+ cache-name: linux-editor-mono
+ target: release_debug
+ tools: true
+ tests: false # Disabled due freeze caused by mix Mono build and CI
+ sconsflags: module_mono_enabled=yes mono_static=yes mono_glue=no
+ doc-test: true
+ bin: "./bin/godot.linuxbsd.opt.tools.64.mono"
+ build-mono: true
+ artifact: true
- name: Editor with doubles and GCC sanitizers (target=debug, tools=yes, float=64, tests=yes, use_asan=yes, use_ubsan=yes)
cache-name: linux-editor-double-sanitizers
@@ -56,14 +56,14 @@ jobs:
# Skip 2GiB artifact speeding up action.
artifact: false
-# - name: Template w/ Mono (target=release, tools=no)
-# cache-name: linux-template-mono
-# target: release
-# tools: false
-# tests: false
-# sconsflags: module_mono_enabled=yes mono_static=yes mono_glue=no debug_symbols=no
-# build-mono: false
-# artifact: true
+ - name: Template w/ Mono (target=release, tools=no)
+ cache-name: linux-template-mono
+ target: release
+ tools: false
+ tests: false
+ sconsflags: module_mono_enabled=yes mono_static=yes mono_glue=no debug_symbols=no
+ build-mono: false
+ artifact: true
- name: Minimal Template (target=release, tools=no, everything disabled)
cache-name: linux-template-minimal
diff --git a/core/object/class_db.cpp b/core/object/class_db.cpp
index d566b0859f..d0fcde832b 100644
--- a/core/object/class_db.cpp
+++ b/core/object/class_db.cpp
@@ -166,15 +166,12 @@ uint64_t ClassDB::get_api_hash(APIType p_api) {
uint64_t hash = hash_djb2_one_64(HashMapHasherDefault::hash(VERSION_FULL_CONFIG));
- List<StringName> names;
+ List<StringName> class_list;
+ ClassDB::get_class_list(&class_list);
+ // Must be alphabetically sorted for hash to compute.
+ class_list.sort_custom<StringName::AlphCompare>();
- for (const KeyValue<StringName, ClassInfo> &E : classes) {
- names.push_back(E.key);
- }
- //must be alphabetically sorted for hash to compute
- names.sort_custom<StringName::AlphCompare>();
-
- for (const StringName &E : names) {
+ for (const StringName &E : class_list) {
ClassInfo *t = classes.getptr(E);
ERR_FAIL_COND_V_MSG(!t, 0, "Cannot get class '" + String(E) + "'.");
if (t->api != p_api || !t->exposed) {
diff --git a/modules/mono/class_db_api_json.cpp b/modules/mono/class_db_api_json.cpp
index 9253f105bb..3afde1e8d3 100644
--- a/modules/mono/class_db_api_json.cpp
+++ b/modules/mono/class_db_api_json.cpp
@@ -40,17 +40,12 @@
void class_db_api_to_json(const String &p_output_file, ClassDB::APIType p_api) {
Dictionary classes_dict;
- List<StringName> names;
+ List<StringName> class_list;
+ ClassDB::get_class_list(&class_list);
+ // Must be alphabetically sorted for hash to compute.
+ class_list.sort_custom<StringName::AlphCompare>();
- const StringName *k = nullptr;
-
- while ((k = ClassDB::classes.next(k))) {
- names.push_back(*k);
- }
- //must be alphabetically sorted for hash to compute
- names.sort_custom<StringName::AlphCompare>();
-
- for (const StringName &E : names) {
+ for (const StringName &E : class_list) {
ClassDB::ClassInfo *t = ClassDB::classes.getptr(E);
ERR_FAIL_COND(!t);
if (t->api != p_api || !t->exposed) {
@@ -66,10 +61,8 @@ void class_db_api_to_json(const String &p_output_file, ClassDB::APIType p_api) {
List<StringName> snames;
- k = nullptr;
-
- while ((k = t->method_map.next(k))) {
- String name = k->operator String();
+ for (const KeyValue<StringName, MethodBind *> &F : t->method_map) {
+ String name = F.key.operator String();
ERR_CONTINUE(name.is_empty());
@@ -77,7 +70,7 @@ void class_db_api_to_json(const String &p_output_file, ClassDB::APIType p_api) {
continue; // Ignore non-virtual methods that start with an underscore
}
- snames.push_back(*k);
+ snames.push_back(F.key);
}
snames.sort_custom<StringName::AlphCompare>();
@@ -131,10 +124,8 @@ void class_db_api_to_json(const String &p_output_file, ClassDB::APIType p_api) {
List<StringName> snames;
- k = nullptr;
-
- while ((k = t->constant_map.next(k))) {
- snames.push_back(*k);
+ for (const KeyValue<StringName, int> &F : t->constant_map) {
+ snames.push_back(F.key);
}
snames.sort_custom<StringName::AlphCompare>();
@@ -158,10 +149,8 @@ void class_db_api_to_json(const String &p_output_file, ClassDB::APIType p_api) {
List<StringName> snames;
- k = nullptr;
-
- while ((k = t->signal_map.next(k))) {
- snames.push_back(*k);
+ for (const KeyValue<StringName, MethodInfo> &F : t->signal_map) {
+ snames.push_back(F.key);
}
snames.sort_custom<StringName::AlphCompare>();
@@ -193,10 +182,8 @@ void class_db_api_to_json(const String &p_output_file, ClassDB::APIType p_api) {
List<StringName> snames;
- k = nullptr;
-
- while ((k = t->property_setget.next(k))) {
- snames.push_back(*k);
+ for (const KeyValue<StringName, ClassDB::PropertySetGet> &F : t->property_setget) {
+ snames.push_back(F.key);
}
snames.sort_custom<StringName::AlphCompare>();
diff --git a/modules/mono/csharp_script.cpp b/modules/mono/csharp_script.cpp
index 75d7c5b92a..31257ac33a 100644
--- a/modules/mono/csharp_script.cpp
+++ b/modules/mono/csharp_script.cpp
@@ -593,9 +593,7 @@ Vector<ScriptLanguage::StackInfo> CSharpLanguage::debug_get_current_stack_info()
return Vector<StackInfo>();
}
_recursion_flag_ = true;
- SCOPE_EXIT {
- _recursion_flag_ = false;
- };
+ SCOPE_EXIT { _recursion_flag_ = false; };
GD_MONO_SCOPE_THREAD_ATTACH;
@@ -627,9 +625,7 @@ Vector<ScriptLanguage::StackInfo> CSharpLanguage::stack_trace_get_info(MonoObjec
return Vector<StackInfo>();
}
_recursion_flag_ = true;
- SCOPE_EXIT {
- _recursion_flag_ = false;
- };
+ SCOPE_EXIT { _recursion_flag_ = false; };
GD_MONO_SCOPE_THREAD_ATTACH;
@@ -1802,8 +1798,8 @@ void CSharpInstance::get_event_signals_state_for_reloading(List<Pair<StringName,
void CSharpInstance::get_property_list(List<PropertyInfo> *p_properties) const {
List<PropertyInfo> props;
- for (HashMap<StringName, PropertyInfo>::ConstElement E = script->member_info.front(); E; E = E.next()) {
- props.push_front(E.value());
+ for (const KeyValue<StringName, PropertyInfo> &E : script->member_info) {
+ props.push_front(E.value);
}
// Call _get_property_list
@@ -3495,8 +3491,8 @@ Ref<Script> CSharpScript::get_base_script() const {
void CSharpScript::get_script_property_list(List<PropertyInfo> *r_list) const {
List<PropertyInfo> props;
- for (HashMap<StringName, PropertyInfo>::ConstElement E = member_info.front(); E; E = E.next()) {
- props.push_front(E.value());
+ for (const KeyValue<StringName, PropertyInfo> &E : member_info) {
+ props.push_front(E.value);
}
for (const PropertyInfo &prop : props) {
diff --git a/modules/mono/editor/bindings_generator.cpp b/modules/mono/editor/bindings_generator.cpp
index 89d2675a4c..168f0254ee 100644
--- a/modules/mono/editor/bindings_generator.cpp
+++ b/modules/mono/editor/bindings_generator.cpp
@@ -1078,8 +1078,8 @@ Error BindingsGenerator::generate_cs_core_project(const String &p_proj_dir) {
compile_items.push_back(output_file);
}
- for (HashMap<StringName, TypeInterface>::Element E = obj_types.front(); E; E = E.next()) {
- const TypeInterface &itype = E.get();
+ for (const KeyValue<StringName, TypeInterface> &E : obj_types) {
+ const TypeInterface &itype = E.value;
if (itype.api_type == ClassDB::API_EDITOR) {
continue;
@@ -1187,8 +1187,8 @@ Error BindingsGenerator::generate_cs_editor_project(const String &p_proj_dir) {
Vector<String> compile_items;
- for (HashMap<StringName, TypeInterface>::Element E = obj_types.front(); E; E = E.next()) {
- const TypeInterface &itype = E.get();
+ for (const KeyValue<StringName, TypeInterface> &E : obj_types) {
+ const TypeInterface &itype = E.value;
if (itype.api_type != ClassDB::API_EDITOR) {
continue;
@@ -1573,9 +1573,9 @@ Error BindingsGenerator::_generate_cs_property(const BindingsGenerator::TypeInte
// Search it in base types too
const TypeInterface *current_type = &p_itype;
while (!setter && current_type->base_name != StringName()) {
- HashMap<StringName, TypeInterface>::Element base_match = obj_types.find(current_type->base_name);
+ HashMap<StringName, TypeInterface>::Iterator base_match = obj_types.find(current_type->base_name);
ERR_FAIL_COND_V_MSG(!base_match, ERR_BUG, "Type not found '" + current_type->base_name + "'. Inherited by '" + current_type->name + "'.");
- current_type = &base_match.get();
+ current_type = &base_match->value;
setter = current_type->find_method_by_name(p_iprop.setter);
}
@@ -1584,9 +1584,9 @@ Error BindingsGenerator::_generate_cs_property(const BindingsGenerator::TypeInte
// Search it in base types too
current_type = &p_itype;
while (!getter && current_type->base_name != StringName()) {
- HashMap<StringName, TypeInterface>::Element base_match = obj_types.find(current_type->base_name);
+ HashMap<StringName, TypeInterface>::Iterator base_match = obj_types.find(current_type->base_name);
ERR_FAIL_COND_V_MSG(!base_match, ERR_BUG, "Type not found '" + current_type->base_name + "'. Inherited by '" + current_type->name + "'.");
- current_type = &base_match.get();
+ current_type = &base_match->value;
getter = current_type->find_method_by_name(p_iprop.getter);
}
@@ -2096,8 +2096,8 @@ Error BindingsGenerator::generate_glue(const String &p_output_dir) {
generated_icall_funcs.clear();
- for (HashMap<StringName, TypeInterface>::Element type_elem = obj_types.front(); type_elem; type_elem = type_elem.next()) {
- const TypeInterface &itype = type_elem.get();
+ for (const KeyValue<StringName, TypeInterface> &type_elem : obj_types) {
+ const TypeInterface &itype = type_elem.value;
bool is_derived_type = itype.base_name != StringName();
@@ -2474,10 +2474,10 @@ const BindingsGenerator::TypeInterface *BindingsGenerator::_get_type_or_null(con
return &builtin_type_match->get();
}
- const HashMap<StringName, TypeInterface>::Element obj_type_match = obj_types.find(p_typeref.cname);
+ const HashMap<StringName, TypeInterface>::Iterator obj_type_match = obj_types.find(p_typeref.cname);
if (obj_type_match) {
- return &obj_type_match.get();
+ return &obj_type_match->value;
}
if (p_typeref.is_enum) {
@@ -2942,12 +2942,11 @@ bool BindingsGenerator::_populate_object_type_interfaces() {
// Populate signals
const HashMap<StringName, MethodInfo> &signal_map = class_info->signal_map;
- const StringName *k = nullptr;
- while ((k = signal_map.next(k))) {
+ for (const KeyValue<StringName, MethodInfo> &E : signal_map) {
SignalInterface isignal;
- const MethodInfo &method_info = signal_map.get(*k);
+ const MethodInfo &method_info = E.value;
isignal.name = method_info.name;
isignal.cname = method_info.name;
@@ -3024,10 +3023,9 @@ bool BindingsGenerator::_populate_object_type_interfaces() {
ClassDB::get_integer_constant_list(type_cname, &constants, true);
const HashMap<StringName, List<StringName>> &enum_map = class_info->enum_map;
- k = nullptr;
- while ((k = enum_map.next(k))) {
- StringName enum_proxy_cname = *k;
+ for (const KeyValue<StringName, List<StringName>> &E : enum_map) {
+ StringName enum_proxy_cname = E.key;
String enum_proxy_name = enum_proxy_cname.operator String();
if (itype.find_property_by_proxy_name(enum_proxy_cname)) {
// We have several conflicts between enums and PascalCase properties,
@@ -3036,7 +3034,7 @@ bool BindingsGenerator::_populate_object_type_interfaces() {
enum_proxy_cname = StringName(enum_proxy_name);
}
EnumInterface ienum(enum_proxy_cname);
- const List<StringName> &enum_constants = enum_map.get(*k);
+ const List<StringName> &enum_constants = E.value;
for (const StringName &constant_cname : enum_constants) {
String constant_name = constant_cname.operator String();
int *value = class_info->constant_map.getptr(constant_cname);
@@ -3066,7 +3064,7 @@ bool BindingsGenerator::_populate_object_type_interfaces() {
TypeInterface enum_itype;
enum_itype.is_enum = true;
- enum_itype.name = itype.name + "." + String(*k);
+ enum_itype.name = itype.name + "." + String(E.key);
enum_itype.cname = StringName(enum_itype.name);
enum_itype.proxy_name = itype.proxy_name + "." + enum_proxy_name;
TypeInterface::postsetup_enum_type(enum_itype);
@@ -3715,8 +3713,8 @@ void BindingsGenerator::_initialize() {
core_custom_icalls.clear();
editor_custom_icalls.clear();
- for (HashMap<StringName, TypeInterface>::Element E = obj_types.front(); E; E = E.next()) {
- _generate_method_icalls(E.get());
+ for (const KeyValue<StringName, TypeInterface> &E : obj_types) {
+ _generate_method_icalls(E.value);
}
initialized = true;
diff --git a/modules/mono/editor/code_completion.cpp b/modules/mono/editor/code_completion.cpp
index e79dd9e4fd..a1789412f4 100644
--- a/modules/mono/editor/code_completion.cpp
+++ b/modules/mono/editor/code_completion.cpp
@@ -123,8 +123,8 @@ PackedStringArray get_code_completion(CompletionKind p_kind, const String &p_scr
// Autoloads.
HashMap<StringName, ProjectSettings::AutoloadInfo> autoloads = ProjectSettings::get_singleton()->get_autoload_list();
- for (HashMap<StringName, ProjectSettings::AutoloadInfo>::Element E = autoloads.front(); E; E = E.next()) {
- const ProjectSettings::AutoloadInfo &info = E.value();
+ for (const KeyValue<StringName, ProjectSettings::AutoloadInfo> &E : autoloads) {
+ const ProjectSettings::AutoloadInfo &info = E.value;
suggestions.push_back(quoted("/root/" + String(info.name)));
}
}
diff --git a/modules/mono/mono_gd/gd_mono.cpp b/modules/mono/mono_gd/gd_mono.cpp
index e98ce8f6c1..39a8ef22b7 100644
--- a/modules/mono/mono_gd/gd_mono.cpp
+++ b/modules/mono/mono_gd/gd_mono.cpp
@@ -1167,9 +1167,8 @@ GDMonoClass *GDMono::get_class(MonoClass *p_raw_class) {
int32_t domain_id = mono_domain_get_id(mono_domain_get());
HashMap<String, GDMonoAssembly *> &domain_assemblies = assemblies[domain_id];
- const String *k = nullptr;
- while ((k = domain_assemblies.next(k))) {
- GDMonoAssembly *assembly = domain_assemblies.get(*k);
+ for (const KeyValue<String, GDMonoAssembly *> &E : domain_assemblies) {
+ GDMonoAssembly *assembly = E.value;
if (assembly->get_image() == image) {
GDMonoClass *klass = assembly->get_class(p_raw_class);
if (klass) {
@@ -1190,9 +1189,8 @@ GDMonoClass *GDMono::get_class(const StringName &p_namespace, const StringName &
int32_t domain_id = mono_domain_get_id(mono_domain_get());
HashMap<String, GDMonoAssembly *> &domain_assemblies = assemblies[domain_id];
- const String *k = nullptr;
- while ((k = domain_assemblies.next(k))) {
- GDMonoAssembly *assembly = domain_assemblies.get(*k);
+ for (const KeyValue<String, GDMonoAssembly *> &E : domain_assemblies) {
+ GDMonoAssembly *assembly = E.value;
klass = assembly->get_class(p_namespace, p_name);
if (klass) {
return klass;
@@ -1205,9 +1203,8 @@ GDMonoClass *GDMono::get_class(const StringName &p_namespace, const StringName &
void GDMono::_domain_assemblies_cleanup(int32_t p_domain_id) {
HashMap<String, GDMonoAssembly *> &domain_assemblies = assemblies[p_domain_id];
- const String *k = nullptr;
- while ((k = domain_assemblies.next(k))) {
- memdelete(domain_assemblies.get(*k));
+ for (const KeyValue<String, GDMonoAssembly *> &E : domain_assemblies) {
+ memdelete(E.value);
}
assemblies.erase(p_domain_id);
@@ -1298,13 +1295,11 @@ GDMono::~GDMono() {
// Leave the rest to 'mono_jit_cleanup'
#endif
- const int32_t *k = nullptr;
- while ((k = assemblies.next(k))) {
- HashMap<String, GDMonoAssembly *> &domain_assemblies = assemblies.get(*k);
+ for (const KeyValue<int32_t, HashMap<String, GDMonoAssembly *>> &E : assemblies) {
+ const HashMap<String, GDMonoAssembly *> &domain_assemblies = E.value;
- const String *kk = nullptr;
- while ((kk = domain_assemblies.next(kk))) {
- memdelete(domain_assemblies.get(*kk));
+ for (const KeyValue<String, GDMonoAssembly *> &F : domain_assemblies) {
+ memdelete(F.value);
}
}
assemblies.clear();
diff --git a/modules/mono/mono_gd/gd_mono_class.cpp b/modules/mono/mono_gd/gd_mono_class.cpp
index 3fc0f16e05..daf70443e9 100644
--- a/modules/mono/mono_gd/gd_mono_class.cpp
+++ b/modules/mono/mono_gd/gd_mono_class.cpp
@@ -247,7 +247,7 @@ void GDMonoClass::fetch_methods_with_godot_api_checks(GDMonoClass *p_native_base
if (existing_method) {
memdelete(*existing_method); // Must delete old one
}
- methods.set(key, method);
+ methods.insert(key, method);
break;
}
@@ -266,11 +266,9 @@ void GDMonoClass::fetch_methods_with_godot_api_checks(GDMonoClass *p_native_base
GDMonoMethod *GDMonoClass::get_fetched_method_unknown_params(const StringName &p_name) {
ERR_FAIL_COND_V(!methods_fetched, nullptr);
- const MethodKey *k = nullptr;
-
- while ((k = methods.next(k))) {
- if (k->name == p_name) {
- return methods.get(*k);
+ for (const KeyValue<MethodKey, GDMonoMethod *> &E : methods) {
+ if (E.key.name == p_name) {
+ return E.value;
}
}
@@ -307,7 +305,7 @@ GDMonoMethod *GDMonoClass::get_method(const StringName &p_name, uint16_t p_param
if (raw_method) {
GDMonoMethod *method = memnew(GDMonoMethod(p_name, raw_method));
- methods.set(key, method);
+ methods.insert(key, method);
return method;
}
@@ -342,7 +340,7 @@ GDMonoMethod *GDMonoClass::get_method(MonoMethod *p_raw_method, const StringName
}
GDMonoMethod *method = memnew(GDMonoMethod(p_name, p_raw_method));
- methods.set(key, method);
+ methods.insert(key, method);
return method;
}
@@ -549,9 +547,8 @@ GDMonoClass::~GDMonoClass() {
Vector<GDMonoMethod *> deleted_methods;
deleted_methods.resize(methods.size());
- const MethodKey *k = nullptr;
- while ((k = methods.next(k))) {
- GDMonoMethod *method = methods.get(*k);
+ for (const KeyValue<MethodKey, GDMonoMethod *> &E : methods) {
+ GDMonoMethod *method = E.value;
if (method) {
for (int i = 0; i < offset; i++) { |
|
There was a bug in Edit: I tested the wrong binary, this isn't solved yet. Digging further. |
akien-mga
approved these changes
May 12, 2022
Adds a new, cleaned up, HashMap implementation. * Uses Robin Hood Hashing (https://en.wikipedia.org/wiki/Hash_table#Robin_Hood_hashing). * Keeps elements in a double linked list for simpler, ordered, iteration. * Allows keeping iterators for later use in removal (Unlike Map<>, it does not do much for performance vs keeping the key, but helps replace old code). * Uses a more modern C++ iterator API, deprecates the old one. * Supports custom allocator (in case there is a wish to use a paged one). This class aims to unify all the associative template usage and replace it by this one: * Map<> (whereas key order does not matter, which is 99% of cases) * HashMap<> * OrderedHashMap<> * OAHashMap<>
|
Thanks! |
|
I just thrown my map benchmark at the new HashMap implementation, and it's significantly slower. It also exhibits a similar issue I got with For the details, this is a benchmark I do in for (int i = 0; i < iterations; ++i) {
ptr += *map.getptr(keys[i % keys.size()]);
}Results are times, but the relevance is in the difference between them. The old HashMap was only about 30% slower than On a logarithmic chart (X: element count, Y: lookup time):
|
Is there's any difference if it's defined as |
|
I defined it as: HashMap<Vec3i, uint64_t, Vec3iHasherGd, HashMapComparatorDefault<Vec3i>> map;With struct Vec3i {
union {
struct {
int x;
int y;
int z;
};
int coords[3];
};
// ...
inline uint32_t hash() const {
uint32_t hash = hash_djb2_one_32(x);
hash = hash_djb2_one_32(y, hash);
hash = hash_djb2_one_32(z, hash);
return hash;
}
};
struct Vec3iHasherGd {
static inline uint32_t hash(const Vec3i &v) {
return v.hash();
}
};Which is basically the same as Godot's |
|
@Zylann Can you try to run the same benchmark on binaries compiled with a recent MinGW version instead of MSVC? Official Godot binaries are compiled using MinGW, so this would be more representative. |
|
Some tests done with 10K, 50K, 100K and 1M insertions and lookups, average from 3 runs:
|
|
Keep in mind a couple of things when comparing:
|
|
Some more tests with the different occupancy (0.75, 0.65, 0.55), also with iteration, deletion, and reinsertion tests and more data points:
|
|
Vec3i lookup benchmark, this time with MinGW. std::unordered_map seems to perform a bit slower here, but still beats the others. The new hashmap still shows the same odd behavior. I also halted the benchmark earlier here because the initial one took an hour to complete because of it, and didn't want to waste more time. |
|
@bruvzg that is interesting, can you try with the following scenarios:
template <class T>
class PagedTypedAllocator {
public:
PagedAllocator<T> allocator;
template <class... Args>
_FORCE_INLINE_ T *new_allocation(const Args &&...p_args) { return allocator.alloc(p_args...); }
_FORCE_INLINE_ void delete_allocation(T *p_allocation) { allocator.free(p_allocation); }
}; |
|
@Zylann I think the inconsistent scaling may have to do with the occupancy value, that should most likely be made smaller. |
|
More tests with template <class T>
class PagedTypedAllocator {
public:
PagedAllocator<T> allocator;
template <class... Args>
_FORCE_INLINE_ T *new_allocation(const Args &&...p_args) { return allocator.alloc(T(p_args...)); }
_FORCE_INLINE_ void delete_allocation(T *p_allocation) { allocator.free(p_allocation); }
};
|
|
@bruvzg Alright, so I was on the right trail with both assumptions:
|
|
@bruvzg that said, thanks so much for running those tests! I think this makes the case that, if needed, we could have an UnorderedHashMap for better performance in the corner cases its really, really needed. |
This was referenced May 16, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a new, cleaned up, HashMap implementation.
This class aims to unify all the associative template usage and replace it by this one:
All usages of HashMap and OrderedHashMap were replaced by the new template (Note: Mono is not working at the moment, pending fix from the Mono team). Subsequent PR will also replace most (if not all) usages of Map<> and Set<> by HashMap and HashSet.