parallelize mesh fixup #1148
parallelize mesh fixup #1148
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1148 +/- ##
==========================================
+ Coverage 91.68% 91.70% +0.01%
==========================================
Files 30 30
Lines 5951 5964 +13
==========================================
+ Hits 5456 5469 +13
Misses 495 495 ☔ View full report in Codecov by Sentry. |
| @@ -223,14 +223,17 @@ void Manifold::Impl::CreateFaces() { | |||
| stable_sort(triPriority.begin(), triPriority.end(), | |||
| [](auto a, auto b) { return a.area2 > b.area2; }); | |||
|
|
|||
| Vec<int> interiorHalfedges; | |||
There was a problem hiding this comment.
It turns out the reallocation is quite significant. I wish the profiler can just give warning when there are many allocator calls within a certain period of time...
|
will try to figure out wtf is gcc complaining about later... |
src/edge_op.cpp
Outdated
| auto expected = std::numeric_limits<size_t>::max(); | ||
| if (!reinterpret_cast<std::atomic<size_t>*>(largestEdge.data() + | ||
| vert) | ||
| ->compare_exchange_strong(expected, largest) && |
There was a problem hiding this comment.
You can just reinterpret a random vector element as an atomic? I guess I'm used to the atomic free functions of CUDA - is this a more idiomatic approach?
There was a problem hiding this comment.
This is not really idiomatic, but it should work. I need to think about the best way to do this.
src/edge_op.cpp
Outdated
| } else { | ||
| endVerts.push_back(halfedge_[current].endVert); | ||
| // switch to hashset for vertices with many neighbors | ||
| if (endVerts.size() > 32) { |
There was a problem hiding this comment.
6 is normal - 10 is already pretty rare. Still, I suppose it's really a perf cross-over.
There was a problem hiding this comment.
Yeah, this is just some random integer that I think should work.
| @@ -204,66 +206,94 @@ void Manifold::Impl::CleanupTopology() { | |||
| // verts. They must be removed before edge collapse. | |||
| SplitPinchedVerts(); | |||
|
|
|||
| Vec<int> entries; | |||
| FlagStore s; | |||
There was a problem hiding this comment.
With your algorithm update, does it still need the while loop? I didn't love having to add that - felt like a cop-out to my algorithm not doing a good enough first pass.
There was a problem hiding this comment.
The algorithm is nearly the same as the old one, just parallelized, so I think the loop is still needed.
SplitPinchedVertsnow runs in parallel.Previously:
Now:
About 10% improvement for larger meshes.