Macaulay2 / mathicgb

Compute (signature) Groebner bases using the fast datastructures from mathic.
2 stars 4 forks source link

TBB 2021 #30

Closed jkyang92 closed 2 years ago

jkyang92 commented 3 years ago

This works for me on Gentoo (TBB 2021) and Arch (TBB 2020). I also tried it with the MATHICGB_NO_TBB flag enabled.

I noticed after I wrote the code that @mikestillman had already made some changes, so feel free to ignore this request if that code is preferred. Mostly since I'd already written this code, I wanted to make it available.

The main change is to use task_arena instead of task_scheduler_init. task_arena seems to be present on the most recent version of TBB 2020, so we should be able to maintain compatibility with most distros, but it'd be great if someone on Ubuntu or Debian can verify.

I only implemented the version of parallel_do that is actually used in mathicgb, since I'd expect that the longer term goal is to get rid of the shim code, and use the new version of TBB directly.

The second commit with the mutex changes isn't needed, but since the TBB 2021 code needs to use std::mutex anyways, it might be good to just use it universally.

mahrud commented 2 years ago

This is nice! Worked for me as well.

mahrud commented 2 years ago

I think if you could try merging with the fix-tbb branch it would be easy to merge it along with @mikestillman's changes.

jkyang92 commented 2 years ago

I think my changes do mostly the same things as that branch. I don't know what mike's plan is, for now I'm mostly maintaining this for my own use, but I'm also happy to create a pull request for the corresponding changes in Macaulay2, but I'm not terribly happy with my solution for that code. The relevant branch for that is this one https://github.com/jkyang92/M2/tree/tbb2021

d-torrance commented 2 years ago

I'm planning on incorporating these changes into the next upload of mathicgb to Debian, since TBB 2021 will likely be uploaded to unstable soon (it's currently in experimental). I had to add the following additional patch to get the autotools build to work:

--- a/src/cli/CommonParams.cpp
+++ b/src/cli/CommonParams.cpp
@@ -60,9 +60,9 @@

   // delete the old init object first to make the new one take control.
   mTbbInit.reset();
-  mTbbInit = make_unique<mgb::mtbb::task_scheduler_init>(
+  mTbbInit = make_unique<mgb::mtbb::task_arena>(
     mThreadCount.value() == 0 ?
-      mgb::mtbb::task_scheduler_init::automatic :
+      mgb::mtbb::task_arena::automatic :
       mThreadCount.value()
   );
 }
--- a/src/cli/CommonParams.hpp
+++ b/src/cli/CommonParams.hpp
@@ -48,7 +48,7 @@
   std::vector<std::string> mExtensions; /// to recognize file type

   /// to set thread count
-  std::unique_ptr<mgb::mtbb::task_scheduler_init> mTbbInit;
+  std::unique_ptr<mgb::mtbb::task_arena> mTbbInit;
   std::size_t mMinDirectParams;
   std::size_t mMaxDirectParams;
   std::vector<std::string> mDirectParameters;
jkyang92 commented 2 years ago

It must be that the cmake build just never builds this file at all. Also task_arena is not a perfect replacement for task_scheduler_init, so to get the thread limits to work correctly you probably need to add a bit more code, I'll take a look and push some changes here later.