Foundation Classes - Performance optimizations and new high-performance collections - #1015
Merged
Merged
Conversation
…ient key-value storage - Introduced NCollection_FlatDataMap, a high-performance hash map using open addressing with Robin Hood hashing, optimizing cache locality and reducing memory allocation overhead. - Added NCollection_FlatMap, a hash set implementation that provides similar benefits for key storage. - Updated OSD_Thread to handle DuplicateHandle failures gracefully. - Enhanced time handling in OSD_Thread::Wait to use nanoseconds for better precision. - Marked Standard_Mutex as deprecated, recommending std::mutex or std::shared_mutex instead. - Improved reference counting in Standard_Transient using atomic operations for thread safety.
…ce collections This commit introduces performance improvements across fundamental OCCT classes and adds new high-performance collection types optimized for modern CPU architectures. New Collection Classes: - NCollection_FlatDataMap: High-performance hash map using open addressing with Robin Hood hashing. Provides better cache locality than NCollection_DataMap by storing all key-value pairs inline in a contiguous array. Features include power-of-2 sizing for fast modulo operations, cached hash codes, exception-safe insertion, and no per-element memory allocations. - NCollection_FlatMap: High-performance hash set with the same optimizations. Matrix and Vector Optimizations: - math_Matrix: Cache-friendly i-k-j loop order for matrix multiplication. The inner loop now accesses matrix rows sequentially, significantly improving cache utilization for large matrices. - math_VectorBase: Norm() and Norm2() rewritten with 4-way loop unrolling enabling better SIMD vectorization. Partial sums are combined pairwise for improved numerical stability. Thread Safety Improvements: - Standard_Transient: Optimized reference counting with explicit memory ordering. IncrementRefCounter uses relaxed ordering (sufficient for pure counting). DecrementRefCounter uses release ordering with an acquire fence only when the count reaches zero, avoiding unnecessary synchronization overhead on every decrement (follows std::shared_ptr pattern). - Standard_Mutex: Deprecated in favor of std::mutex. Added deprecation warnings indicating removal in OCCT 8.0.0. Bug Fixes: - OSD_Thread (Windows): Added error handling for DuplicateHandle failure in Assign() method, properly resetting handle and thread ID on failure. - OSD_Thread (POSIX): Fixed nanoseconds overflow in Wait() when the computed timeout exceeds 1 second. Added normalization to properly carry excess nanoseconds to seconds. Tests: - Added comprehensive GTest suites for NCollection_FlatDataMap and NCollection_FlatMap covering basic operations, iterators, edge cases, collisions, and performance characteristics.
dpasukhi
added
2. Enhancement
There was a problem hiding this comment.
Pull request overview
This pull request introduces performance optimizations across OCCT's foundation classes, adding two new high-performance collection types (NCollection_FlatDataMap and NCollection_FlatMap) and optimizing existing mathematical operations. The changes also include thread safety improvements through explicit memory ordering in reference counting, deprecation of Standard_Mutex in favor of std::mutex, and bug fixes for OSD_Thread error handling.
Changes:
- New flat hash map/set implementations using Robin Hood hashing for improved cache locality
- Optimized matrix multiplication and vector norm calculations with loop unrolling and improved cache access patterns
- Enhanced thread safety in Standard_Transient with explicit memory ordering
- Bug fixes for Windows handle duplication and POSIX timeout overflow
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Standard_Transient.hxx | Updated reference counting to use explicit memory ordering (relaxed for increment, release/acquire for decrement) |
| Standard_Mutex.hxx | Added deprecation warnings indicating removal in OCCT 8.0.0 |
| OSD_Thread.cxx | Added error handling for DuplicateHandle failure and fixed nanoseconds overflow in POSIX Wait() |
| NCollection_FlatMap.hxx | New high-performance hash set implementation using Robin Hood hashing |
| NCollection_FlatDataMap.hxx | New high-performance hash map implementation using Robin Hood hashing |
| NCollection/FILES.cmake | Registered new FlatDataMap and FlatMap header files |
| NCollection_FlatMap_Test.cxx | Added comprehensive test suite for NCollection_FlatMap |
| NCollection_FlatDataMap_Test.cxx | Added comprehensive test suite for NCollection_FlatDataMap |
| GTests/FILES.cmake | Registered new test files |
| math_VectorBase.lxx | Optimized Norm() and Norm2() with 4-way loop unrolling |
| math_Matrix.lxx | Optimized matrix multiplication with cache-friendly i-k-j loop order |
| gp_XYZ.hxx | Updated to use std::sqrt and improved comment formatting |
Comment on lines
+25
to
+26
| #include <cmath> | ||
|
|
There was a problem hiding this comment.
The include of <cmath> is unnecessary. The file already uses the unqualified sqrt() function from the global namespace (implicitly available via other headers). Since line 156 changes to std::sqrt(), this include should either be removed (if sqrt is available from existing headers) or the existing code should verify it was working without it before. The change from sqrt to std::sqrt and adding <cmath> should be done together only if needed.
Suggested change
| #include <cmath> |
| //! Computes std::sqrt(X*X + Y*Y + Z*Z) where X, Y and Z are the three coordinates of this XYZ | ||
| //! object. | ||
| double Modulus() const { return sqrt(x * x + y * y + z * z); } | ||
| double Modulus() const { return std::sqrt(x * x + y * y + z * z); } |
There was a problem hiding this comment.
If std::sqrt is now required (line 156), ensure the <cmath> header include on line 25 is necessary. However, the original code used unqualified sqrt() which was likely provided by existing headers. Verify if adding the explicit std:: qualifier and <cmath> include are both necessary, or if this introduces redundancy.
| { | ||
| public: | ||
| //! STL-compliant typedef for key type | ||
| typedef TheKeyType key_type; |
Collaborator
There was a problem hiding this comment.
Why do we use typedef instead of using?
Comment on lines
+64
to
+66
| static constexpr uint8_t SLOT_EMPTY = 0; | ||
| static constexpr uint8_t SLOT_DELETED = 1; | ||
| static constexpr uint8_t SLOT_USED = 2; |
Collaborator
There was a problem hiding this comment.
It seems like this could be enum class instead of just a bunch of constants.
Also, maybe you could provide a description for these markers? It is kinda unclear what's the difference between empty and deleted states and what it affects.
| } | ||
| } | ||
|
|
||
| bool findSlot(const TheKeyType& theKey, size_t& theIndex) const |
Collaborator
There was a problem hiding this comment.
Wouldn't it be better to use std::optional here instead?
…Collection_FlatMap and NCollection_FlatDataMap
dpasukhi
added a commit
to dpasukhi/OCCT
that referenced
this pull request
Jan 21, 2026
…Open-Cascade-SAS#1015) Add new methods to NCollection map classes for conditional binding and in-place construction, following STL conventions while maintaining OCCT naming patterns. New methods added: NCollection_Map: - Emplace(Args...) - construct key in-place, returns bool - Emplaced(Args...) - construct key in-place, returns const Key& NCollection_DataMap: - TryBind/TryBound - bind only if key doesn't exist - Emplace/Emplaced - construct value in-place - TryEmplace/TryEmplaced - construct value in-place only if key doesn't exist NCollection_IndexedMap: - Added(Key) - add and return const reference to key - Emplace/Emplaced - construct key in-place NCollection_IndexedDataMap: - TryBound - bind only if key doesn't exist, returns reference - Bind/Bound - bind with overwrite semantics (moved from Add behavior) - Emplace/Emplaced - construct value in-place - TryEmplace/TryEmplaced - construct value in-place only if key doesn't exist NCollection_DoubleMap: - TryBind - bind only if neither key exists (no exception) - TryEmplace - same as TryBind with forwarding - Added move overloads for Bind method NCollection_FlatMap: - Added(Key) - add and return const reference to key - Emplace/Emplaced - construct key in-place NCollection_FlatDataMap: - TryBind/TryBound - bind only if key doesn't exist - Bound - bind with overwrite, returns reference - Emplace/Emplaced - construct value in-place - TryEmplace/TryEmplaced - construct value in-place only if key doesn't exist Implementation details: - Used template helpers with tag dispatch (std::bool_constant) for code reuse - Used if constexpr for compile-time branching between Try/non-Try variants - Used std::conditional_t for methods returning different types - Perfect forwarding with std::forward for all key/value parameters - Move assignment for overwrites (clean, exception-safe pattern) - Added in_place constructor to NCollection_TListNode for Emplace support Bug fixes: - NCollection_FlatMap/FlatDataMap: Fixed memory safety issues with Slot storage - Changed Slot to use uninitialized storage (alignas char[]) for keys/items - Properly manage key/item lifetimes with explicit constructor/destructor calls - Fixed backwardShiftDelete to properly destroy moved-from objects - NCollection_IndexedMap/IndexedDataMap: Fixed exception safety issue - Moved Increment() call after successful node construction to prevent size corruption if constructor throws Test coverage: - Added comprehensive GTests for all new methods - Tests cover: new key insertion, existing key behavior, move semantics, reference validity, and comparison with existing methods
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
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.
This commit introduces performance improvements across fundamental OCCT classes
and adds new high-performance collection types optimized for modern CPU architectures.
New Collection Classes:
Robin Hood hashing. Provides better cache locality than NCollection_DataMap
by storing all key-value pairs inline in a contiguous array. Features include
power-of-2 sizing for fast modulo operations, cached hash codes, exception-safe
insertion, and no per-element memory allocations.
Matrix and Vector Optimizations:
The inner loop now accesses matrix rows sequentially, significantly
improving cache utilization for large matrices.
enabling better SIMD vectorization. Partial sums are combined pairwise for
improved numerical stability.
Thread Safety Improvements:
IncrementRefCounter uses relaxed ordering (sufficient for pure counting).
DecrementRefCounter uses release ordering with an acquire fence only when
the count reaches zero, avoiding unnecessary synchronization overhead on
every decrement (follows std::shared_ptr pattern).
indicating removal in OCCT 8.0.0.
Bug Fixes:
Assign() method, properly resetting handle and thread ID on failure.
timeout exceeds 1 second. Added normalization to properly carry excess
nanoseconds to seconds.
Tests:
NCollection_FlatMap covering basic operations, iterators, edge cases,
collisions, and performance characteristics.