Skip to content

Hash_map: Changed Unique_hash_map implementation to use CGAL unordered_flat_map#9444

Open
GilesBathgate wants to merge 8 commits intoCGAL:mainfrom
GilesBathgate:Hash_map-performance_unordered_flat_map-GilesBathgate
Open

Hash_map: Changed Unique_hash_map implementation to use CGAL unordered_flat_map#9444
GilesBathgate wants to merge 8 commits intoCGAL:mainfrom
GilesBathgate:Hash_map-performance_unordered_flat_map-GilesBathgate

Conversation

@GilesBathgate
Copy link
Copy Markdown
Contributor

@GilesBathgate GilesBathgate commented Apr 21, 2026

Summary of Changes

  • Changed Unique_hash_map.h to use CGAL::unordered_flat_map as its backend.
  • Removed original chained_map.h

The map when using boost::unordered_flat_map uses cache friendly memory layout. The mulx mixing uses Fibonacci hashing, leading to a ~6-7% performance boost in the Nef_3_benchmark.

Release Management

  • Affected package(s): Hash_map, Nef_3...
  • Issue(s) solved (if any): performance
  • License and copyright ownership: CGAL

@GilesBathgate GilesBathgate changed the title Changed Unique_hash_map implementation to use CGAL unordered_flat_map Hash_mal: Changed Unique_hash_map implementation to use CGAL unordered_flat_map Apr 21, 2026
@GilesBathgate GilesBathgate changed the title Hash_mal: Changed Unique_hash_map implementation to use CGAL unordered_flat_map Hash_map: Changed Unique_hash_map implementation to use CGAL unordered_flat_map Apr 21, 2026
Comment thread Miscellany/doc/Miscellany/CGAL/Unique_hash_map.h Outdated
@afabri
Copy link
Copy Markdown
Member

afabri commented Apr 27, 2026

As the underlying unordered flat map enables to remove we could add a remove() method. The CGAL::Unique_hash_map does not have that, and that's why it was fast. I am wondering if something else makes it special and why we not just replace and deprecate.

@GilesBathgate
Copy link
Copy Markdown
Contributor Author

GilesBathgate commented Apr 27, 2026

As the underlying unordered flat map enables to remove we could add a remove() method.

We could but why not just call that erase for STL style compatability?

Likewise add a contains and deprecate is_defined?

The CGAL::Unique_hash_map does not have that, and that's why it was fast.

Not sure thats why it was fast, std::unordered_map has erase and uses a chained approach. Although I don't know if the scheme links into a block of data at the end of the table as per the LEDA implementation. I created a draft for chained map erase: #6493

Either way, flat maps seem to be the modern faster approach. Adding an erase would be useful for SNC_structure which uses a std::optional<Object_handle> to store "erased" handles in the map.

@afabri
Copy link
Copy Markdown
Member

afabri commented Apr 28, 2026

If I understand correctly here we only have a fast under the hood implementation for a sufficiently recent version of Boost. Users with an older version will have a std::map as fallback. That seems not a good idea to me.
Can you please double check @sloriot or @lrineau or @MaelRL

@afabri
Copy link
Copy Markdown
Member

afabri commented Apr 28, 2026

@afabri Yes in that case with a compile time check it could fall back to the chained map, or my flat_map in the other PR.

Part of the understanding problem is that I am not sure if the other PR's got replaced or are still open, and if they are in a specific order to apply.

@GilesBathgate
Copy link
Copy Markdown
Contributor Author

GilesBathgate commented Apr 28, 2026

Users with an older version will have a std::map as fallback.

I think despite the misleading comment, it actually falls back to boost::unordered_map instead of boost::unordered_flat_map

#else // Boost before 1.81.0, use the C++11 std::unordered_map
# include <boost/unordered_map.hpp>
# include <boost/unordered_set.hpp>
#endif

std::map is only used when CGAL_USE_BARE_STD_MAP is defined. I have created a branch with code to fall back to linear_map but I think its unneeded and would introduce maintaince overhead.

@afabri
Copy link
Copy Markdown
Member

afabri commented Apr 29, 2026

Warning concerning the documentation here

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.

3 participants