remove deterministic flag #1033
Conversation
|
I see. I think the For the other change, can you check if removing it will cause major slowdown? |
|
Some additional context: Due to things like floating point, and changes to mesh simplification, enabling the deterministic version will not give you the same result across different versions or with different machines. I think this also makes deterministic result less important for our use case because we can't have that in general, and not sure if users demanding deterministic result are happy with only this same version guarantee (if it holds at all... we never actually tested it anyway). I think we can just remove the flag and keep the things that may cause nondeterminism, and try to deal with them later when someone complains. |
|
Another reason: Floating point operations are not really associative, and making this deterministic will require a lot more changes. The topology may be deterministic previously with the deterministic flag on, but things like normal and positions are probably not. If users want deterministic results, they should probably run with parallelization disabled (either compile with that or limit to 1 CPU core). |
| @@ -253,7 +253,7 @@ void AddNewEdgeVerts( | |||
| #if (MANIFOLD_PAR == 1) && __has_include(<tbb/tbb.h>) | |||
| // parallelize operations, requires concurrent_map so we can only enable this | |||
| // with tbb | |||
| if (!ManifoldParams().deterministic && p1q2.size() > kParallelThreshold) { | |||
| if (p1q2.size() > kParallelThreshold) { | |||
There was a problem hiding this comment.
@pca006132 Actually, switching this to serial had the smallest performance impact of anything, but for now I'm going with the determinism == false path for everything.
| SharedImpl r; | ||
| queue.try_pop(r); | ||
| return *std::get_if<std::shared_ptr<Manifold::Impl>>(&r); | ||
| tbb::task_group group; |
There was a problem hiding this comment.
This one had a noticeable perf impact when removed, but not huge.
| ? ExecutionPolicy::Par | ||
| : ExecutionPolicy::Seq, | ||
| impl->children_.begin(), impl->children_.end(), [](auto &child) { | ||
| for_each(ExecutionPolicy::Par, impl->children_.begin(), |
There was a problem hiding this comment.
Changing this to Seq caused about 75% of the performance regression I was seeing. Even just the impl->children_.size() > 1 check was causing noticeable perf regression.
|
maybe we can document this in https://github.com/elalish/manifold/wiki/Performance-Considerations |
I want to remove the
deterministicflag from our execution parameters - it leaves untested code paths that concern me. The question is what is more important: determinism or speed? I'm not quite ready to merge this as is because forcing everything to be deterministic all the time is making our tests run about 20% slower (at least on my M1 macbook pro). WDYT @pca006132?