Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a new HashSet template #61194

Merged
merged 1 commit into from
May 20, 2022
Merged

Add a new HashSet template #61194

merged 1 commit into from
May 20, 2022

Conversation

reduz
Copy link
Member

@reduz reduz commented May 19, 2022

  • Intended to replace RBSet in most cases.
  • Optimized for iteration speed

@reduz reduz requested review from a team as code owners May 19, 2022 15:01
@KoBeWi KoBeWi added this to the 4.0 milestone May 19, 2022
core/templates/hash_set.h Outdated Show resolved Hide resolved
tests/test_main.cpp Show resolved Hide resolved
@bruvzg
Copy link
Member

bruvzg commented May 19, 2022

Some tests:

Raw data

Average of 10 runs.

Container Nr of iterations Insert (µs) Lookup (µs) Iterate (µs) Erase (µs)
HashSet 10000 178 14 15 21
HashSet 50000 848 97 73 141
HashSet 250000 4519 579 455 862
HashSet 1000000 21052 3663 1790 6688
HashSet 5000000 200373 36725 9166 54732
RBSet 10000 713 386 17 397
RBSet 50000 4266 2042 89 2114
RBSet 250000 24769 11218 527 12005
RBSet 1000000 121293 47999 1914 51400
RBSet 5000000 723722 274974 10721 294013
std::set 10000 525 356 33 737
std::set 50000 2858 1886 170 3554
std::set 250000 18098 10522 940 20897
std::set 1000000 79986 45284 3998 90211
std::set 5000000 422946 236493 18821 469186
unordered_set 10000 240 21 13 196
unordered_set 50000 899 112 66 1018
unordered_set 250000 5385 607 336 5483
unordered_set 1000000 34054 8335 1414 42392
unordered_set 5000000 284808 78297 6756 356546

Screenshot 2022-05-19 at 18 53 06

@reduz
Copy link
Member Author

reduz commented May 19, 2022

@lawnjelly props seems your algorithm idea was pretty good!

@bruvzg what is the weirdest here is that iteration is faster in the STL one. in HashSet iteration is literally just an array!

@mrjustaguy
Copy link
Contributor

mrjustaguy commented May 19, 2022

Eh, the graphs aren't marked.. what is time and what is the number of executions?
I'm guessing time (ms?) is the y axis and the x axis the number of executions. is that right?

@bruvzg
Copy link
Member

bruvzg commented May 19, 2022

I'm guessing time (ms?) is the y axis and the x axis the number of executions. is that right?

Y - time (µs, log scale), X - number of iterations.

@bruvzg
Copy link
Member

bruvzg commented May 20, 2022

what is the weirdest here is that iteration is faster in the STL one. in HashSet iteration is literally just an array!

LLVM black magic? When it's built with GCC it's not faster.

The previous test was with LLVM 13, this one with GCC 11.3:

Screenshot 2022-05-20 at 12 16 42

And MSVC 17.2:

Screenshot 2022-05-20 at 13 24 01

Probably it's doing some low-level optimizations, like autovectorization that's can't be easily done with the custom iterator.

@lawnjelly
Copy link
Member

lawnjelly commented May 20, 2022

@lawnjelly props seems your algorithm idea was pretty good!

As always, I would like to take credit when it goes right, and when it goes wrong it was all your idea. 😀

I would actually be tempted to go further and try fully decoupling the two resizes (the keys versus the actual hashmap list). The keys could be stored as a vector and resized with a x2 basis (which reduces the resizes of the key, which could be expensive if they are big).

EDIT: Ah I now see the hash_table_size_primes that you added. This is already near x2 so scratch the earlier comment. Although I'm not sure this has any negative implications for the open addressing. I guess it just uses more memory than the old approach, so maybe it is okay, but might need some scrutiny just in case there are side effects with caching with the larger jump (not necessarily in this template but maybe in hashmap).

One thing I noticed is that although the non-pot might reduce hash collisions, it means you have to use a % operator instead of just a bitwise AND to wraparound. Hash collisions phasing is probably super important to avoid but worth a mention, I don't know whether all use cases are susceptible to this phasing.

Also it might be an idea to try templating the uint32_t size for the IDs, because if you can guarantee the size will be less than 65535 you can use uint16_t which may be faster (may not of course, have to benchmark 😁 ).

As I say I think this approach of separating the expensive elements and using an ID in the hashmap list may also work well for the unordered hashmap.

@reduz reduz requested review from a team as code owners May 20, 2022 12:13
@reduz reduz requested review from a team as code owners May 20, 2022 12:13
@reduz reduz force-pushed the new-hash-set branch 4 times, most recently from 5a9da0a to d7c294e Compare May 20, 2022 16:29
@reduz reduz requested a review from a team as a code owner May 20, 2022 16:29
@akien-mga
Copy link
Member

Leak sanitizer is detecting lots of leaks (in ShaderLanguage?), see https://github.com/godotengine/godot/runs/6528386338?check_suite_focus=true

* Intended to replace RBSet in most cases.
* Optimized for iteration speed
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, early benchmarks seem promising. I did a quick sanity check that editor startup and rendering of a simple 3D scene didn't regress (same speed as before).

@akien-mga akien-mga merged commit 8adf048 into godotengine:master May 20, 2022
@akien-mga
Copy link
Member

Thanks!

@Calinou
Copy link
Member

Calinou commented May 21, 2022

For reference, here's a benchmark of editor startup/shutdown on an empty project:

❯ hyperfine -m20 -iw1 "bin/godot.linuxbsd.tools.64.llvm.old /tmp/x/project.godot --quit" "bin/godot.linuxbsd.tools.64.llvm /tmp/x/project.godot --quit"
Benchmark #1: bin/godot.linuxbsd.tools.64.llvm.old /tmp/x/project.godot --quit
  Time (mean ± σ):      8.412 s ±  0.298 s    [User: 7.112 s, System: 0.358 s]
  Range (min … max):    7.533 s …  8.627 s    20 runs
 
Benchmark #2: bin/godot.linuxbsd.tools.64.llvm /tmp/x/project.godot --quit
  Time (mean ± σ):      8.337 s ±  0.276 s    [User: 7.100 s, System: 0.354 s]
  Range (min … max):    7.929 s …  8.600 s    20 runs
 
Summary
  'bin/godot.linuxbsd.tools.64.llvm /tmp/x/project.godot --quit' ran
    1.01 ± 0.05 times faster than 'bin/godot.linuxbsd.tools.64.llvm.old /tmp/x/project.godot --quit'

@reduz
Copy link
Member Author

reduz commented May 21, 2022

@Calinou all these changes show that, despite criticism that we were not using the most efficient data structures in most cases, in the end (now that we are) it makes little to no difference because these structures were not used for critical areas.

pafuent added a commit to pafuent/godot that referenced this pull request Oct 16, 2024
Fixes godotengine#97066

`RBSet` were used on `RotatedFileLogger` because it guarantees that
iterating it is done via `operator<`. This is important because
`RotatedFileLogger` depends on this behavior to delete the oldest log file.
On godotengine#61194 `HashSet` was added and all `RBSet` uses were replaced by
`HashSet`.
When that happened, the iteration in order is guaranteed to be the insertion
order, wich made that `RotatedFileLogger` delete the newest log file.
As a bonus, I added unit test for `RotatedFileLogger` and `CompositeLogger`.
pafuent added a commit to pafuent/godot that referenced this pull request Oct 17, 2024
Fixes godotengine#97066

`RBSet` were used on `RotatedFileLogger` because it guarantees that
iterating it is done via `operator<`. This is important because
`RotatedFileLogger` depends on this behavior to delete the oldest log file.
On godotengine#61194 `HashSet` was added and all `RBSet` uses were replaced by
`HashSet`.
When that happened, the iteration in order is guaranteed to be the insertion
order, wich made that `RotatedFileLogger` delete the newest log file.
As a bonus, I added unit test for `RotatedFileLogger` and `CompositeLogger`.
pafuent added a commit to pafuent/godot that referenced this pull request Nov 13, 2024
Fixes godotengine#97066

`RBSet` were used on `RotatedFileLogger` because it guarantees that
iterating it is done via `operator<`. This is important because
`RotatedFileLogger` depends on this behavior to delete the oldest log file.
On godotengine#61194 `HashSet` was added and all `RBSet` uses were replaced by
`HashSet`.
When that happened, the iteration in order is guaranteed to be the insertion
order, wich made that `RotatedFileLogger` delete the newest log file.
As a bonus, I added unit test for `RotatedFileLogger` and `CompositeLogger`.
pafuent added a commit to pafuent/godot that referenced this pull request Nov 13, 2024
Fixes godotengine#97066

`RBSet` were used on `RotatedFileLogger` because it guarantees that
iterating it is done via `operator<`. This is important because
`RotatedFileLogger` depends on this behavior to delete the oldest log file.
On godotengine#61194 `HashSet` was added and all `RBSet` uses were replaced by
`HashSet`.
When that happened, the iteration in order is guaranteed to be the insertion
order, wich made that `RotatedFileLogger` delete the newest log file.
As a bonus, I added unit test for `RotatedFileLogger` and `CompositeLogger`.
pafuent added a commit to pafuent/godot that referenced this pull request Nov 20, 2024
Fixes godotengine#97066

`RBSet` were used on `RotatedFileLogger` because it guarantees that
iterating it is done via `operator<`. This is important because
`RotatedFileLogger` depends on this behavior to delete the oldest log file.
On godotengine#61194 `HashSet` was added and all `RBSet` uses were replaced by
`HashSet`.
When that happened, the iteration in order is guaranteed to be the insertion
order, wich made that `RotatedFileLogger` delete the newest log file.
As a bonus, I added unit test for `RotatedFileLogger` and `CompositeLogger`.
pafuent added a commit to pafuent/godot that referenced this pull request Nov 29, 2024
Fixes godotengine#97066

`RBSet` were used on `RotatedFileLogger` because it guarantees that
iterating it is done via `operator<`. This is important because
`RotatedFileLogger` depends on this behavior to delete the oldest log file.
On godotengine#61194 `HashSet` was added and all `RBSet` uses were replaced by
`HashSet`.
When that happened, the iteration in order is guaranteed to be the insertion
order, wich made that `RotatedFileLogger` delete the newest log file.
As a bonus, I added unit test for `RotatedFileLogger` and `CompositeLogger`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants