fix performance regression for CreateProperties

[pca006132]

Closes #619.

For some reason the proposed fix by @stephomi caused wrong index calculation and caused invalid index access later. I just do the minimum required to fix the performance issue, and it seems that it is not too slow. Perf doesn't report CreateProperties as a bottleneck.

The test case can check the performance regression. Basically, without this fix, the performance is so slow that I am not willing to wait for it to complete. It now takes 1200ms to complete.

[pca006132]

While debugging this I also discovered another bug that cases the test to fail when circularSegments is set to 4096. There is no overflow. I am debugging to see why...

[stephomi]

Yeah, the proposal was completely broken.
You can you try again with the new version, it should be fine.

diff --git a/src/manifold/src/boolean_result.cpp b/src/manifold/src/boolean_result.cpp
index 08ca4af..6a2529c 100644
--- a/src/manifold/src/boolean_result.cpp
+++ b/src/manifold/src/boolean_result.cpp
@@ -533,6 +533,7 @@ void CreateProperties(Manifold::Impl &outR, const Manifold::Impl &inP,
   using Entry = std::pair<glm::ivec3, int>;
   int idMissProp = outR.NumVert();
   std::vector<std::vector<Entry>> propIdx(outR.NumVert() + 1);
+  std::vector<int> propMissIdx(outR.NumVert() * 2, -1);
   outR.meshRelation_.properties.reserve(outR.NumVert() * numProp);
   int idx = 0;
 
@@ -577,17 +578,27 @@ void CreateProperties(Manifold::Impl &outR, const Manifold::Impl &inP,
         }
       }
 
-      auto &bin = propIdx[key.y];
-      bool bFound = false;
-      for (int k = 0; k < bin.size(); ++k) {
-        if (bin[k].first == glm::ivec3(key.x, key.z, key.w)) {
-          bFound = true;
-          outR.meshRelation_.triProperties[tri][i] = bin[k].second;
-          break;
+      if (key.y == idMissProp && key.z >= 0) {
+        // only key.x/key.z matters
+        auto &entry = propMissIdx[key.z * 2 + key.x];
+        if (entry >= 0) {
+          outR.meshRelation_.triProperties[tri][i] = entry;
+          continue;
         }
+        entry = idx;
+      } else {
+        auto &bin = propIdx[key.y];
+        bool bFound = false;
+        for (int k = 0; k < bin.size(); ++k) {
+          if (bin[k].first == glm::ivec3(key.x, key.z, key.w)) {
+            bFound = true;
+            outR.meshRelation_.triProperties[tri][i] = bin[k].second;
+            break;
+          }
+        }
+        if (bFound) continue;
+        bin.push_back(std::make_pair(glm::ivec3(key.x, key.z, key.w), idx));
       }
-      if (bFound) continue;
-      bin.push_back(std::make_pair(glm::ivec3(key.x, key.z, key.w), idx));
       outR.meshRelation_.triProperties[tri][i] = idx++;
 
       for (int p = 0; p < numProp; ++p) {

[pca006132]

@stephomi No it still doesn't work, with similar error as the previous patch.
Also, I think the solution using hashmap is simple and efficient enough?

[pca006132]

While debugging this I also discovered another bug that cases the test to fail when circularSegments is set to 4096. There is no overflow. I am debugging to see why...

OK I was wrong, there is in fact overflow. I will add some checks to check for overflow.

[pca006132]

The SparseIndices too large error is caused by me forgetting to add a translate for the sphere union test, so everything collided, and the spheres with 4096 circular segments are pretty large... It is now checked to report an error earlier.

[stephomi]

Not efficient enough to me, a vector of hashmap is meh..
I'll see what's wrong with my proposal (I only tested with my code, I didn't checked the manifold test)

[pca006132]

I wonder if the problem is due to the hashmap or the overall algorithm. Btw, maybe you can add some test cases to show the performance issue when you have time?

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (2e29784) 91.41% compared to head (98b12b3) 91.40%.
Report is 1 commits behind head on master.

Files	Patch %	Lines
src/utilities/include/sparse.h	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #621      +/-   ##
==========================================
- Coverage   91.41%   91.40%   -0.02%     
==========================================
  Files          35       35              
  Lines        4497     4512      +15     
==========================================
+ Hits         4111     4124      +13     
- Misses        386      388       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stephomi]

It adds +15% extra time to the whole boolean operation.

I wonder if the problem is due to the hashmap or the overall algorithm

I only tested in my codebase, I didn't run the manifold test yet.
I was ignoring the case of oldNumProp <=0, I fixed the snippet it should be good (hopefully).

---

(right now the manifold CMake fails on my machine, so I can't run the test... but I don't really want to investigate at the moment, CMake gets on my nerve, it's probably the FetchContent thing)

The link interface of target "utilities" contains:

    glm::glm

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

[pca006132]

@stephomi thanks, it works now. So it seems that most of the bins contain 0/1 entries, and the one corresponds to idMissProp can contain many entries but are addressable using key.z * 2 + key.x? This is nice.

[pca006132]

hmm, it seems that it is not correct yet.

[pca006132]

seems that w is also important?

[stephomi]

If w is set then y equals vert (and I assume vert is always < idMissProp).
I'm a bit in a blind since I can't run tests locally atm.

[pca006132]

indeed it makes sense, I should look into it more closely to see why it breaks...

[pca006132]

it seems that triProperties can be at most 3 * numTri? So the maximum value of key.z is 3*numTri.

[stephomi]

Ah indeed, I used the wrong length.
I was stupidly testing boolean with both the same mesh (same number of vertices...).

I think it should be std::vector<int> propMissIdx(std::max(inP.NumPropVert(), inQ.NumPropVert()) * 2, -1);

Or we can also do

  std::vector<int> propMissIdx[2];
  propMissIdx[0].resize(inQ.NumPropVert(), -1);
  propMissIdx[1].resize(inP.NumPropVert(), -1);
  // [...]
  auto &entry = propMissIdx[key.x][key.z];

[pca006132]

done

[elalish]

Excellent, nice pair programming!