start removing thrust

[pca006132]

Start implementing #809

Some blockers:

1. copy_if with stencil.
2. transform_reduce but it should be easy to work around.
3. reduce_by_key.

and some utilities provided by thrust.

[elalish]

What's the trouble with transform_reduce? https://en.cppreference.com/w/cpp/algorithm/transform_reduce

Looks like copy_if we can just convert the stencil to a predicate. reduce_by_key is only used once; I don't think we'll lose much if we implement it serially.

Should I make those changes in this branch?

[pca006132]

What's the trouble with transform_reduce? https://en.cppreference.com/w/cpp/algorithm/transform_reduce

Oh, for some reason I was not aware of that. Will fix.

Should I make those changes in this branch?

Sure

[elalish]

I took a look at this - algorithmically it seems very close. However, do you think you can get it to a state where it compiles? Perhaps just make some stubs for the unimplemented functions? Though countAt() would be nice to have. And do you think we can now remove all the #defines from par.h?

I can get it all back to running once it compiles, but I'm not sure how to step in at the current state.

[pca006132]

I forgot to remove those thrust functions and call their single threaded variant when pstl is not available. I think it should be easy to fix, will do it later this week after my paper author response period.

[pca006132]

I am trying to work on the zip iterator thing, I hate c++ templates... hopefully it will be done by this weekend.

[pca006132]

I tried to switch to std::tuple for zip_iterator, but it seems that this will not work well with std::sort where it attempts to swap temporaries (https://stackoverflow.com/questions/21588477/swap-temporary-tuples-of-references). I think we still need our own tuple wrapper, and it may look a bit ugly, as I need to do some recursive template stuff to implement variable-length tuples.

[elalish]

Can we borrow from Thrust's zip iterator / tuple? They have a compatible license. And if not the code itself, perhaps just inspiration...

[pca006132]

They are also using their own tuple type, and the implementation is also complicated. Probably same amount of work.

[pca006132]

Will probably need more time to deal with the tuple stuff. @elalish this should now be in good shape for you to work on.

[elalish]

Okay, thanks!

[elalish]

It looks like with LLVM on my mac, I'm failing this pstl check, which is why I commented out the std::copy_if(std::execution::par_unseq stuff below. Is there a more generic way to check for the presence of PSTL?

[elalish]

Also, it appears LLVM is missing copy_if for PSTL in general, not just the stencil version.

[elalish]

@pca006132 I think this is back to you for iterators now. Let me know if there's anything else I can help with.

[codecov]

Codecov Report

Attention: Patch coverage is 97.05882% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.00%. Comparing base (d437097) to head (85e2d64).
Report is 56 commits behind head on master.

Files	Patch %	Lines
src/utilities/include/par.h	94.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #823      +/-   ##
==========================================
- Coverage   91.84%   90.00%   -1.84%     
==========================================
  Files          37       64      +27     
  Lines        4976     9073    +4097     
  Branches        0      952     +952     
==========================================
+ Hits         4570     8166    +3596     
- Misses        406      907     +501     

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

[elalish]

Okay, it looks like libc++ is missing PSTL versions of copy_if, uninitialized_fill, uninitialized_copy, and remove_if. I'm getting a linker error locally with my last commit that I don't understand.

[pca006132]

Can you show me the linker error? I think the other errors on the CI are quite easy to understand. I don't have time to fix them now though, helping other students in our group to implement some experiment...

[elalish]

Sure, it's weird that I don't see the same error on the mac CI:

[build] Undefined symbols for architecture arm64:
[build]   "std::__1::__par_backend::__libdispatch::__dispatch_apply(unsigned long, void*, void (*)(void*, unsigned long))", referenced from:
[build]       manifold::TriangulateIdx(std::__1::vector<std::__1::vector<manifold::PolyVert, std::__1::allocator<manifold::PolyVert>>, std::__1::allocator<std::__1::vector<manifold::PolyVert, std::__1::allocator<manifold::PolyVert>>>> const&, float) in polygon.cpp.o
[build]       manifold::SparseIndices manifold::Collider::Collisions<true, false, manifold::Box>(manifold::VecView<manifold::Box const> const&) const in collider.cpp.o
[build]       manifold::SparseIndices manifold::Collider::Collisions<true, false, manifold::Box>(manifold::VecView<manifold::Box const> const&) const in collider.cpp.o
[build]       manifold::SparseIndices manifold::Collider::Collisions<false, false, manifold::Box>(manifold::VecView<manifold::Box const> const&) const in collider.cpp.o
[build]       manifold::SparseIndices manifold::Collider::Collisions<false, false, manifold::Box>(manifold::VecView<manifold::Box const> const&) const in collider.cpp.o
[build]       manifold::SparseIndices manifold::Collider::Collisions<false, false, glm::vec<3, float, (glm::qualifier)0>>(manifold::VecView<glm::vec<3, float, (glm::qualifier)0> const> const&) const in collider.cpp.o
[build]       manifold::SparseIndices manifold::Collider::Collisions<false, false, glm::vec<3, float, (glm::qualifier)0>>(manifold::VecView<glm::vec<3, float, (glm::qualifier)0> const> const&) const in collider.cpp.o
[build]       ...
[build]   "std::__1::__par_backend::__libdispatch::__partition_chunks(long)", referenced from:
[build]       manifold::TriangulateIdx(std::__1::vector<std::__1::vector<manifold::PolyVert, std::__1::allocator<manifold::PolyVert>>, std::__1::allocator<std::__1::vector<manifold::PolyVert, std::__1::allocator<manifold::PolyVert>>>> const&, float) in polygon.cpp.o
[build]       manifold::SparseIndices manifold::Collider::Collisions<true, false, manifold::Box>(manifold::VecView<manifold::Box const> const&) const in collider.cpp.o
[build]       manifold::SparseIndices manifold::Collider::Collisions<true, false, manifold::Box>(manifold::VecView<manifold::Box const> const&) const in collider.cpp.o
[build]       manifold::SparseIndices manifold::Collider::Collisions<false, false, manifold::Box>(manifold::VecView<manifold::Box const> const&) const in collider.cpp.o
[build]       manifold::SparseIndices manifold::Collider::Collisions<false, false, manifold::Box>(manifold::VecView<manifold::Box const> const&) const in collider.cpp.o
[build]       manifold::SparseIndices manifold::Collider::Collisions<false, false, glm::vec<3, float, (glm::qualifier)0>>(manifold::VecView<glm::vec<3, float, (glm::qualifier)0> const> const&) const in collider.cpp.o
[build]       manifold::SparseIndices manifold::Collider::Collisions<false, false, glm::vec<3, float, (glm::qualifier)0>>(manifold::VecView<glm::vec<3, float, (glm::qualifier)0> const> const&) const in collider.cpp.o
[build]       ...
[build] ld: symbol(s) not found for architecture arm64

[elalish]

Not finding much online for this error, except this hilariously snarky forum about how awful Homebrew is (which I use - pretty unclear what the alternative would be?). It's never been a problem before; I sure hope it's not now.

[elalish]

@pca006132 My last commit is clearly not right - somehow it's failing on both mac and non-mac now, but I figured you might like to see what I tried. If you go back one commit, you'll see it passed the CI, so that may be a better place to start.

[pca006132]

The error is just

g++-13: error: unrecognized command-line option ‘-fexperimental-library’

Should be easy to only add this when compiling on mac.

[elalish]

Fair, but the mac CI is also giving me this: error: no member named 'execution' in namespace 'std'; which makes me think I'm doing something more seriously wrong. Especially since I seem to get past that error locally, but then hit the linker problem.

I'm going to start a separate PR to work on removing our fancy iterator usage. I really don't know what I'm doing when it comes to getting PSTL to compile on different architectures.

[elalish]

I'm reverting my failed CMake changes so that we can at least get the CI passing. I have no idea how to get the PSTL to compile on my M1 Macbook. Any experience with this @kintel?

[kintel]

I'm reverting my failed CMake changes so that we can at least get the CI passing. I have no idea how to get the PSTL to compile on my M1 Macbook. Any experience with this @kintel?

I don't see any failures. Do you have an example I can use to make it break?

[elalish]

@kintel The PR here is only using serial execution for me, rather than any PSTL on my M1. I tried to fix that with the commit I reverted above, but I got a weird libdispatch linker error. I'm curious if that is just due to some local homebrew screwup on my machine or if my approach is unsound. Can you check if that commit builds on your M1?

[kintel]

@kintel The PR here is only using serial execution for me, rather than any PSTL on my M1. I tried to fix that with the commit I reverted above, but I got a weird libdispatch linker error. I'm curious if that is just due to some local homebrew screwup on my machine or if my approach is unsound. Can you check if that commit builds on your M1?

I have Manifold built from 67e9d05. Do I need any special build flags to reproduce the issue?

[elalish]

I have Manifold built from 67e9d05. Do I need any special build flags to reproduce the issue?

No, just MANIFOLD_PAR=TBB. If you can build, that's already better than me. If it also runs at the usual parallel speed instead of being slowed down to single-threaded, then we'd be in business.

[kintel]

I just did a git clone, checkout hash, cmake and build. I guess I was lucky? Is there a test which tells me good vs. bad build? Like an expensive test?

[elalish]

Just run the whole suite with test/manifold_test. It usually runs in 35-40 seconds on my M1, but over 100 if parallelization is disabled. You can always build with MANIFOLD_PAR=NONE to compare.

I guess it's a good sign that it builds - perhaps it is just my machine that's in a bad state. Still, let's make sure it's actually running in parallel properly first, since my builds fine for serial already.

[kintel]

Meh, my build wasn't parallel. Will try again tomorrow

[kintel]

Looks like Apple is still on clang 15, which doesn't implement std::execution.

[kintel]

Workarounds exist, like https://github.com/mikekazakov/pstld

[elalish]

Dang, I thought we had researched this already, but apparently not. I guess even Clang's partial PSTL support is only available in probably 17 or 18. That pstld is interesting, though I'm a little concerned by how unused it seems to be as a project. Then I found this, which makes me wonder if linking in oneDPL on mac would be a good approach. @pca006132 what do you think?

[pca006132]

Should be fine. I don't have particular preference or knowledge for compiling on Mac :)

[elalish]

I removed my top few commits - I moved the commit that removed reduce_by_key over to #852 since it also helped with zip removal. I tried merging master on this branch, but it failed to compile the Python bindings, so I aborted. Figure you'll know what's going on there.

[elalish]

Okay, I figured out the weird python binding problem - somehow pulling master was creating a bad merge that reverted our nanobind submodule version (??). Once I got that reverted it's now building fine.

[pca006132]

Ah, maybe you did not run submodule update and commited the wrong version of nanobind? No idea how this happens... Anyway, it is nice that the issue is fixed.

[pca006132]

Closing in flavor of #856. We may still want to cherry-pick stuff from here later.