Foundation Classes - Align FlatMap/FlatDataMap lookup path and update usage notes - #1108
Conversation
… usage notes - Simplify `findSlotIndex()` in `NCollection_FlatMap` and `NCollection_FlatDataMap` to probe until empty slot or key match. - Remove lookup early-exit based on probe distance to keep lookup behavior consistent between both flat containers. - Reorder `NCollection_FlatDataMap::Slot` members to keep hash/probe metadata before key/value storage. - Refresh class-level Doxygen comments with practical usage guidance and relative notes vs `NCollection_Map` / `NCollection_DataMap`.
dpasukhi
added
the
1. Foundation Classes
There was a problem hiding this comment.
Pull request overview
This pull request simplifies the lookup path in NCollection_FlatMap and NCollection_FlatDataMap by removing the Robin Hood early-exit optimization and updates the class-level documentation with clearer usage guidance. The changes align the lookup behavior between both flat containers and improve documentation clarity.
Changes:
- Simplified
findSlotIndex()to probe until finding an empty slot or key match, removing the early-exit optimization based on probe distance comparison - Reordered
NCollection_FlatDataMap::Slotstruct members to place hash/probe metadata before key/value storage for better cache locality - Enhanced Doxygen documentation with practical usage patterns, performance characteristics relative to
NCollection_Map/NCollection_DataMap, and implementation notes
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| NCollection_FlatMap.hxx | Simplified lookup loop and updated documentation with usage patterns and relative performance notes |
| NCollection_FlatDataMap.hxx | Simplified lookup loop, reordered Slot members, and updated documentation with usage patterns and relative performance notes |
| size_t aIndex = aHash & aMask; | ||
|
|
||
| while (aProbe < aMaxProbe) | ||
| while (true) |
There was a problem hiding this comment.
Removing the Robin Hood early-exit optimization (probe distance check) will significantly degrade lookup performance in pathological cases with high collision rates. In the worst case (e.g., with ConstantHasher that returns the same hash for all keys), a failed lookup must now scan through all occupied slots until finding an empty one, rather than exiting early when encountering a slot with lower probe distance. This could mean scanning hundreds of consecutive slots instead of just a few.
While the load factor guarantee ensures the loop will terminate, the performance degradation is measurable. Consider keeping the early-exit optimization for better performance characteristics, especially since Robin Hood hashing is designed to maintain the invariant that enables this optimization.
| size_t myHash; //!< Cached hash code | ||
| //! Distance from ideal bucket plus one; 0 means Empty, otherwise Used. | ||
| size_t myProbeDistancePlus1; | ||
| alignas(TheKeyType) char myKeyStorage[sizeof(TheKeyType)]; | ||
| alignas(TheItemType) char myItemStorage[sizeof(TheItemType)]; |
There was a problem hiding this comment.
The reordering of Slot struct members to place hash/probe metadata before key/value storage is a good optimization. This improves cache locality during lookups since the hash and probe distance are checked before accessing the key. However, verify that this doesn't introduce unexpected padding that increases the overall Slot size. Consider documenting the rationale for this ordering in a comment.
| size_t aIndex = aHash & aMask; | ||
|
|
||
| while (aProbe < aMaxProbe) | ||
| while (true) |
There was a problem hiding this comment.
Removing the Robin Hood early-exit optimization (probe distance check) will significantly degrade lookup performance in pathological cases with high collision rates. In the worst case (e.g., with ConstantHasher that returns the same hash for all keys), a failed lookup must now scan through all occupied slots until finding an empty one, rather than exiting early when encountering a slot with lower probe distance. This could mean scanning hundreds of consecutive slots instead of just a few.
While the load factor guarantee ensures the loop will terminate, the performance degradation is measurable. Consider keeping the early-exit optimization for better performance characteristics, especially since Robin Hood hashing is designed to maintain the invariant that enables this optimization.
findSlotIndex()inNCollection_FlatMapandNCollection_FlatDataMapto probe until empty slot or key match.NCollection_FlatDataMap::Slotmembers to keep hash/probe metadata before key/value storage.NCollection_Map/NCollection_DataMap.