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

Replace most uses of Map by HashMap #60999

Merged
merged 1 commit into from
May 16, 2022

Conversation

reduz
Copy link
Member

@reduz reduz commented May 13, 2022

  • Map is unnecessary and inefficient in almost every case.
  • Replaced by the new HashMap
  • Renamed Map to RBMap and RBSet for cases that still make sense (order matters) but use is discouraged.

There were very few cases where replacing by HashMap was undesired because keeping the key order was intended. I tried to keep those (as RBMap) as much as possible, but might have missed some. Review appreciated!

Bugsquad edit: Follow-up to #60881.

@reduz reduz requested review from a team as code owners May 13, 2022 13:28
@reduz reduz force-pushed the replace-rbhash-by-hashmap branch 6 times, most recently from 7c9ce88 to 523aa82 Compare May 14, 2022 14:41
@reduz
Copy link
Member Author

reduz commented May 14, 2022

This is now ready for review.

* Map is unnecessary and inefficient in almost every case.
* Replaced by the new HashMap.
* Renamed Map to RBMap and Set to RBSet for cases that still make sense
  (order matters) but use is discouraged.

There were very few cases where replacing by HashMap was undesired because
keeping the key order was intended.
I tried to keep those (as RBMap) as much as possible, but might have missed
some. Review appreciated!
@akien-mga
Copy link
Member

Amended to fix (manually..) the broken formatting caused by clang-format 14 in some headers.

@akien-mga
Copy link
Member

Did a quick sanity check as I had done for #60881. As mentioned there, it's not necessarily representative of what this PR impacts, it's just to make sure there's no huge obvious regression. The test is to start the editor on a 3D scene and then close it.

$ hyperfine -iw2 -m10 "bin/godot.linuxbsd.opt.tools.64.map_to_hashmap /home/akien/Projects/godot/projects/liblast/Game/project.godot" "bin/godot.linuxbsd.opt.tools.64.orig /home/akien/Projects/godot/projects/liblast/Game/project.godot"
Benchmark #1: bin/godot.linuxbsd.opt.tools.64.map_to_hashmap /home/akien/Projects/godot/projects/liblast/Game/project.godot
  Time (mean ± σ):     10.125 s ±  0.397 s    [User: 7.405 s, System: 0.479 s]
  Range (min … max):    9.458 s … 10.654 s    10 runs
 
Benchmark #2: bin/godot.linuxbsd.opt.tools.64.orig /home/akien/Projects/godot/projects/liblast/Game/project.godot
  Time (mean ± σ):      9.822 s ±  0.480 s    [User: 7.117 s, System: 0.491 s]
  Range (min … max):    9.016 s … 10.374 s    10 runs
 
Summary
  'bin/godot.linuxbsd.opt.tools.64.orig /home/akien/Projects/godot/projects/liblast/Game/project.godot' ran
    1.03 ± 0.06 times faster than 'bin/godot.linuxbsd.opt.tools.64.map_to_hashmap /home/akien/Projects/godot/projects/liblast/Game/project.godot'

The difference seems small enough to merge this for now and unblock other PRs.

There's likely still room for optimization and for doing proper benchmarks to see if/where there are bottlenecks. But generally speaking this PR mostly refactors the codebase to a better API so it can be merged as is, and the base HashMap / RBSet / RBMap implementations can be optimized further without changing the rest of the codebase.

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Seems to run fine on macOS, and I do not see any obvious slowdowns.

Note: There are a few questionable include changes (maprb_map while code was changed to use HashMap).

@akien-mga
Copy link
Member

Note: There are a few questionable include changes (map → rb_map while code was changed to use HashMap).

We might want to do a cleanup pass for rb_map.h and rb_set.h by comparing the outputs of:

rg RBMap -l | sort
rg rb_map.h -l | sort

@akien-mga akien-mga merged commit c1277c2 into godotengine:master May 16, 2022
@akien-mga
Copy link
Member

Thanks!

@lufog
Copy link
Contributor

lufog commented May 16, 2022

@reduz, this PR broke class_names.
ClassNameTest_G4.zip (in v4.0.alpha.custom_build [396def9] works fine)

Edit:
I just noticed that there is already a bug report for this (#61084).

Copy link

@Brayan-724 Brayan-724 left a comment

Choose a reason for hiding this comment

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

.

Meptl added a commit to Meptl/goost that referenced this pull request May 9, 2023
OrderedHashMap -> HashMap
Map -> HashMap

See godot commit 8b7c7f5a753b43cec10f72b274bb1d70c253652b
godotengine/godot#60999
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