-
Notifications
You must be signed in to change notification settings - Fork 32
Use axom::FlatMap in spin's Octree implementation #1614
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
Conversation
| #else | ||
| using MapType = std::unordered_map<RepresentationType, BroodDataType>; | ||
| #endif | ||
| using MapType = axom::FlatMap<RepresentationType, BroodDataType>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This is the main change in this PR -- using axom::FlatMap instead of google::dense_hash_map or std::unordered_map in SparseOctreeLevel
| IteratorHelper(OctreeLevelType* octLevel, bool begin) | ||
| : m_offset(0) | ||
| : m_currentIter(begin ? octLevel->m_map.begin() : octLevel->m_map.end()) | ||
| , m_offset(0) | ||
| , m_isLevelZero(octLevel->level() == 0) | ||
| { | ||
| m_currentIter = begin ? octLevel->m_map.begin() : octLevel->m_map.end(); | ||
| } | ||
| { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@publixsubfan -- FlatMap was mostly a drop-in replacement for the other hash maps.
The only part that needed adjustment was in this constructor. It seems that the iterator for FlatMap does not have a default constructor.
Without this change, I got lots of errors of the form
In file included from <axom>/src/tools/data_collection_util.cpp:12:
In file included from <axom>/build_axom/include/axom/quest.hpp:12:
In file included from <axom>/src/axom/quest/Delaunay.hpp:14:
In file included from <axom>/build_axom/include/axom/spin.hpp:17:
In file included from <axom>/src/axom/spin/OctreeBase.hpp:22:
<axom>/src/axom/spin/SparseOctreeLevel.hpp:157:5: error: constructor for 'axom::spin::SparseOctreeLevel<3, axom::quest::InOutBlockData, unsigned int>::IteratorHelper<const axom::spin::SparseOctreeLevel<3, axom::quest::InOutBlockData, unsigned int>, axom::FlatMap<unsigned int, axom::NumericArray<axom::quest::InOutBlockData, 8>>::IteratorImpl<true>, axom::spin::OctreeLevel<3, axom::quest::InOutBlockData>::ConstBlockIteratorHelper>' must explicitly initialize the member 'm_currentIter' which does not have a default constructor
IteratorHelper(OctreeLevelType* octLevel, bool begin)
^
<axom>/src/axom/spin/SparseOctreeLevel.hpp:220:16: note: in instantiation of member function 'axom::spin::SparseOctreeLevel<3, axom::quest::InOutBlockData, unsigned int>::IteratorHelper<const axom::spin::SparseOctreeLevel<3, axom::quest::InOutBlockData, unsigned int>, axom::FlatMap<unsigned int, axom::NumericArray<axom::quest::InOutBlockData, 8>>::IteratorImpl<true>, axom::spin::OctreeLevel<3, axom::quest::InOutBlockData>::ConstBlockIteratorHelper>::IteratorHelper' requested here
return new ConstIterHelper(this, begin);
^
<axom>/src/axom/spin/SparseOctreeLevel.hpp:202:3: note: in instantiation of member function 'axom::spin::SparseOctreeLevel<3, axom::quest::InOutBlockData, unsigned int>::getIteratorHelper' requested here
SparseOctreeLevel(int level = -1) : Base(level) { BroodTraits::initializeMap(m_map); }
^
<axom>/src/axom/spin/OctreeBase.hpp:453:35: note: in instantiation of member function 'axom::spin::SparseOctreeLevel<3, axom::quest::InOutBlockData, unsigned int>::SparseOctreeLevel' requested here
m_leavesLevelMap[i] = new Sparse32OctLevType(i);
^
<axom>/src/axom/spin/SpatialOctree.hpp:48:7: note: in instantiation of member function 'axom::spin::OctreeBase<3, axom::quest::InOutBlockData>::OctreeBase' requested here
: BaseOctree()
^
<axom>/src/axom/quest/InOutOctree.hpp:166:7: note: in instantiation of member function 'axom::spin::SpatialOctree<3, axom::quest::InOutBlockData>::SpatialOctree' requested here
: SpatialOctreeType(GeometricBoundingBox(bb).scale(DEFAULT_BOUNDING_BOX_SCALE_FACTOR))
^
<axom>/src/axom/quest/detail/shaping/InOutSampler.hpp:92:20: note: in instantiation of member function 'axom::quest::InOutOctree<3>::InOutOctree' requested here
m_octree = new InOutOctreeType(m_bbox, m_surfaceMesh);
^
<axom>/src/axom/quest/SamplingShaper.hpp:176:25: note: in instantiation of member function 'axom::quest::shaping::InOutSampler<3>::initSpatialIndex' requested here
m_inoutSampler3D->initSpatialIndex(this->m_vertexWeldThreshold);
^
<axom>/src/axom/spin/SparseOctreeLevel.hpp:195:21: note: member is declared here
AdaptedIterType m_currentIter;
^
<axom>/src/axom/core/FlatMap.hpp:671:42: note: 'axom::FlatMap<unsigned int, axom::NumericArray<axom::quest::InOutBlockData, 8>>::IteratorImpl<true>' declared here
class FlatMap<KeyType, ValueType, Hash>::IteratorImpl
^
i.e.: error: constructor for 'ConstBlockIteratorHelper' must explicitly initialize the member 'm_currentIter' which does not have a default constructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that looks like a bug in FlatMap: the LegacyForwardIterator concept also requires DefaultConstructible https://en.cppreference.com/w/cpp/named_req/ForwardIterator.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created #1618 to track this.
| struct PointHash | ||
| { | ||
| using MortonIndex = std::size_t; | ||
| using result_type = std::size_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This was the only other change to make axom::FlatMap work with the PointHash class!
(i.e. adding an expected typedef)
| /** | ||
| * \brief Generate the spatial index over the surface mesh | ||
| */ | ||
| /// \brief Generate the spatial index over the surface mesh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for all the noise in this PR -- I used it as an opportunity to clean up documentation in the files related to the InOutOctree
| //Returns 30 bit morton code for coordinates point is expected to be between [0,1] | ||
| template <typename FloatType, int Dims> | ||
| static inline AXOM_HOST_DEVICE std::int32_t morton32_encode(const primal::Vector<FloatType, Dims>& point) | ||
| static inline AXOM_HOST_DEVICE std::uint32_t morton32_encode(const primal::Vector<FloatType, Dims>& point) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another minor change -- the return type for these morton_encode functions should be unsigned since the MortonIndexType is unsigned.
For other recent changes to this function, see https://github.com/LLNL/axom/pull/1611/files#diff-17b5f2106d61093b9348d431dc0e84ce494f4551984015540561e7ebf53c0e32
While looking at this again, I decided to use axom::clampVal instead of fmin(fmax, ...)
|
@kennyweiss based on the performance results, is your plan to use sparsehash for CPU implementation and flatmap for GPU? That would seem to be an easy configuration option since flatmap is a drop-in replacement for sparshehash. Would sparsehash need to be enhanced to be thread safe for OpenMP parallelism, for example? |
|
Thanks for the SSE improvements @publixsubfan! Here are my updated results for release configs (note the different y-axis ranges): There's a definite improvement, but there's still a performance regression w.r.t. sparsehash. Here are the comparisons |
@rhornung67 -- While it would be relatively easy to add support back for the sparsehash in this PR, it will become much more difficult to support both once we start using the batch creation features from FlatMap (#1610), and I'm anticipating the latter (and associated algorithmic changes) to yield significant speedups in construction times. I spoke to a few key users of the InOutOctree and they are ok with the temporary performance regression as we port this to the GPU. For now, I'll note the regression in the RELEASE-NOTES. |
|
@kennyweiss how does performance compare with sparsehash using @publixsubfan SIMD approach? |
I posted it in the charts above. The SSE intrinsics get us part of the way there. There's now about 10-20% slowdown in construction and about 20% slowdown in queries (with some exceptions). |
Misc: Includes some minor doc updates and code moderization
If we want to reuse a timer, we need to reset it.
BradWhitlock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR seems fine as it is mostly comment reformatting, enum->constexpr changes, and minor changes needed to use FlatMap.
We are working on fixes for these.
bde6639 to
57b1128
Compare




Summary
axom::FlatMapin the implementation ofspin::SparseOctreeLevelPerformance
Here is the performance difference I saw using
axom::FlatMapinstead of sparsehash in the implementationin the InOutOctree
containment_driverexample.Notes/observations:
std:unordered_mapandsparsehashsparsehashfor construction and queries