Codecov Report

Attention: Patch coverage is 79.00000% with 21 lines in your changes missing coverage. Please review.

Project coverage is 90.04%. Comparing base (d437097) to head (036091a).
Report is 57 commits behind head on master.

@@            Coverage Diff             @@
##           master     #856      +/-   ##
==========================================
- Coverage   91.84%   90.04%   -1.80%     
==========================================
  Files          37       65      +28     
  Lines        4976     9069    +4093     
  Branches        0      946     +946     
==========================================
+ Hits         4570     8166    +3596     
- Misses        406      903     +497     

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

OK thrust is now gone in this branch. We have some performance penalty, but can probably fix that via some tuning? idk

I always find it annoying how Git sees one PR built on top of another PR as a giant pile of merge conflicts... You'd think they could resolve this automatically when the top commits have been merged to master.

fixing now

And just so we know where to look - where have you noticed a performance penalty?

I need some more time to do rigorous profiling. I think I just noticed the unit test time increased by a bit, not much though.

After running the tests again, it seems that we are roughly 4% faster, not slower, regardless of whether MANIFOLD_DEBUG is on or off. Maybe my feeling that this is slower is due to some random thermo thing for my computer.

and we probably want to add the cmake config needed to check pstl version? for #787.

Okay, I've got OneDPL linked in now and I'm getting the same compiler problem on the CI as my local Mac, so that should aid with debug a bit. I have no idea what this means:

/opt/homebrew/include/oneapi/dpl/internal/../pstl/glue_numeric_impl.h:43:12: error: call to function 'transform_reduce' that is neither visible in the template definition nor found by argument-dependent lookup
    return transform_reduce(::std::forward<_ExecutionPolicy>(__exec), __first, __last, __init, __binary_op,
           ^
/Users/runner/work/manifold/manifold/src/utilities/include/par.h:132:21: note: in instantiation of function template specialization 'oneapi::dpl::reduce<const oneapi::dpl::execution::parallel_unsequenced_policy &, glm::vec<3, float> *, glm::vec<3, float>, (lambda at /Users/runner/work/manifold/manifold/src/manifold/src/properties.cpp:329:58)>' requested here
STL_DYNAMIC_BACKEND(reduce, void)
                    ^
/Users/runner/work/manifold/manifold/src/manifold/src/properties.cpp:327:15: note: in instantiation of function template specialization 'manifold::reduce<glm::vec<3, float>, glm::vec<3, float> *, glm::vec<3, float> *, glm::vec<3, float>, (lambda at /Users/runner/work/manifold/manifold/src/manifold/src/properties.cpp:329:58)>' requested here
  bBox_.min = reduce<glm::vec3>(
              ^
/opt/homebrew/include/oneapi/dpl/pstl/glue_numeric_defs.h:60:1: note: 'transform_reduce' should be declared prior to the call site or in an associated namespace of one of its arguments
transform_reduce(_ExecutionPolicy&& __exec, _ForwardIterator __first, _ForwardIterator __last, _Tp __init,
^

One the plus side, it is actually trying to use the PSTL from DPL, I just don't know what it doesn't like.

I also don't understand all of the known limitations, or if we might be hitting any of them: https://oneapi-src.github.io/oneDPL/introduction.html#difference-with-standard-c-parallel-algorithms

are they just reduce and transform_reduce? can you get it working by changing them to sequential for the time being? just to see if oneDPL works.

Good call, it was only reduce and transform_reduce - it's now running faster on my M1 than before this PR (~15% faster, not bad!). I suppose parallel reduce isn't too critical, but I'd love to understand the problem.

Okay, now the Mac CI is just failing the cmake consumer test. What's the proper way to handle the OneDPL dependency? Do we link it in test-cmake.sh, or change something about the CMake install?

This is the closest I've come to useful info about the reduce error. It almost seems like it's a OneDPL bug? Unless there's something about our lambdas that are confusing it.

This is the closest I've come to useful info about the reduce error. It almost seems like it's a OneDPL bug? Unless there's something about our lambdas that are confusing it.

Feels like so. Maybe you can try to install oneDPL from source with the latest version and see if it works? I think we should probably file an upsteam issue if this persists.

Also, I wonder if we need PSTL at all. Most functions here are rather simple to implement with tbb parallel_for and parallel_reduce, the only exception is probably stable_sort?

and doing that will fix #787 as well. but probably a bit too much for today.

@elalish things should work now.

Also, I wonder if we need PSTL at all. Most functions here are rather simple to implement with tbb parallel_for and parallel_reduce, the only exception is probably stable_sort?

I see the appeal, but in the name of maintainability, PSTL feels safer long term. Maybe something better than TBB comes along, like TBB overtook OpenMP? Maybe new architectures come along that need a new parallel driver (e.g. Google TPUs)? I feel like PSTL will have the best chance of supporting all these unknown futures with minimal effort on our part. But that said, I'm open to being convinced otherwise.