fresh rebase of threading for DAGMC #561

makeclean · 2018-09-17T21:35:06Z

This finally adds thread support to DAGMC, this includes the most efficient method of creating a new DAGMC constructor that allows reuse of the GTT that way there is no duplication of OBB's. This is neede for @pshriwise for OpenMC work.

As ever all comments welcome :)

Also, please let me know the new requirements for PR's

pshriwise · 2018-09-17T21:37:28Z

Thank you @makeclean!! This is going to be extremely helpful.

One new thing we do is add a short blurb file (basically the PR description) to the news directory.

I'll take a look at the code changes soon!

pshriwise · 2018-09-18T14:36:15Z

Code style needs an update and it looks this is relying on some std library calls that don't exist in older compiler versions.

makeclean · 2018-09-18T14:39:36Z

I will PR those changes later tonight :)

pshriwise

I have a few questions, but this looks great @makeclean!

pshriwise · 2018-09-18T15:00:09Z

src/dagmc/CMakeLists.txt

 list(APPEND DAGMC_SRC_FILES ${CMAKE_SOURCE_DIR}/src/uwuw/uwuw.cpp)

 # Public headers
 list(APPEND DAGMC_PUB_HEADERS DagMC.hpp)
 list(APPEND DAGMC_PUB_HEADERS dagmcmetadata.hpp)
+list(APPEND DAGMC_PUB_HEADERS thread_manager.hpp)


If this is going to be a public header, we might want to get DAGMC or DAG into its name to avoid clashing with another header in the future.

pshriwise · 2018-09-18T15:04:36Z

src/dagmc/DagMC.cpp

-  delete GTT;
+  if(gtt_instance_created) 
+    delete GTT;
+
  delete GQT;


We can always delete the GQT safely too?

pshriwise · 2018-09-18T15:05:07Z

src/dagmc/DagMC.hpp

@@ -30,6 +29,10 @@ namespace moab {

 class CartVect;

+#define DAGMC_VERSION 3.0


These are no longer included from the DagMCVersion header?

Thats either a new one on me, or one I was never aware of. Should I just include DagMCVersion.hpp?

pshriwise · 2018-09-18T15:06:49Z

src/dagmc/test/CMakeLists.txt

@@ -26,6 +26,15 @@ setup_test(dagmc_pointinvol_test cpp ${DRIVER} "${LINK_LIBS}")
 setup_test(dagmc_rayfire_test    cpp ${DRIVER} "${LINK_LIBS}")
 setup_test(dagmc_simple_test     cpp ${DRIVER} "${LINK_LIBS}")

+if(OPENMP_BUILD)


Is OPENMP_BUILD a DAGMC CMake variable? Should we add it as an option in the main CMakeLists file?

Since the threadmanager does not itself depend on OpenMP this was only so that the test actually uses it. So I'll defer on that one, all the -D options are all global anyhow, so if you set

cmake -DOPENMP_BUILD=ON .....

Then its throughout all the Cmakefiles anyhow

pshriwise · 2018-09-18T15:08:20Z

src/dagmc/test/dagmc_unit_test_driver.cc

+
+  // set the number of threads
+  if ( argc > 1 ) {
+    num_thread_to_run = std::atoi(argv[1]);


This is that line causing some of the unit tests to fail.

Ok, that one can be fixed.

pshriwise · 2018-09-18T15:26:26Z

src/dagmc/thread_manager.cpp

+}
+
+// set the number of threads
+void DagThreadManager::set_num_threads(int thread_count) {


I was a little confused by this. Is this something that might change often?

no, but I through habit always give a public member function to change stuff through the constructor. Like you could call with 1 thread to the main constructor, the use case I had in mind was that at the time of loading the DAGMC geometry, one didnt know the number of threads. Some time later, you figure out the number of threads, you can then set the number of threads with that function and then setup accordingly.

I see what you mean. There should probably be some way to handle going from N to M threads, deleting or constructing instances as needed, yeah?

The case where M < N is probably not very likely, but tackling the general problem early will mean it's taken care of for good. Could there be a method called update_child_threads which would handle this?

TBH I only really imagined from going from 1->M, I hadnt thought about going down, but yes to do it properly one should have an update method.

pshriwise · 2018-09-18T15:32:11Z

One thing I was wondering about the interface is if we want to replicate some of the DAGMC calls via the interface, but with an additional thread argument at the end. So DAGMC->ray_fire(vol, pos, dir, dist, surf)
would become DAGMC->ray_fire(vol, pos, dir, dist, surf, thread_id) in the threaded interface.

Additionally, it might be nice to have a constructor which will create all of the DAGMC instances, setup the OBB tree, and initialize all instances in one step, so it looks very much like our current DAGMC constructor, but, again, with an additional argument indicating the number of threads.

makeclean · 2018-09-18T18:35:03Z

re the constructor it's bad practise to have operations that can fail, so setting up obbs can fail hence why it's done outside the constructor now. Unless something has changed and I don't realise it? Right now we construct, load_file and then init_OBBs after right? If what you want really is to mask the cruft of setting up threads, I can have a think how that could be done.

pshriwise · 2018-09-18T20:19:43Z

True about having potential failure points in the constructor.

Yeah I think that's mainly what I'm going for. In the case of what I'll call "thread 0" operations, those could be wrapped automatically to the 0th DAGMC instance.

moab::ErrorCode DagMCThreadManager::init_OBBs() { return dagmc_instances[0]->init_OBBs(); }

makeclean · 2018-09-18T20:31:29Z

I see, so you want the thread manager functions to more closely mimic the main DagMC interface?

pshriwise · 2018-09-18T20:34:37Z

Wherever possible, yeah. Users can still access DAGMC instances manually with what's there already. Tricky parts may involve places in MC interface code where we interface directly with RayHistory objects - doing resets and such. I'm thinking of MCNP here.

But I suppose there's no reason we couldn't forward those calls to the right object as well.

makeclean · 2018-09-18T20:52:19Z

Hence the macros

#define DAGMC(X) DTM->get_dagmc_instance(X)
#define RAY_HISTORY(X) DTM->get_ray_history(X)

That's what the macros are intended to simplify. e.g.

RAY_HISTORY(omp_get_thread_num()).reset();
RAY_HISTORY(omp_get_thread_num()).rollback();

Is that insufficient? Or is it more that there needs to be a neater way? I mean you could actually change a whole bunch of stuff around. So for example

DTM *DAGMC = new DagThreadManager();
DAGMC->ray_fire(...,ray_history(thread_id), thread_id);
DAGMC->point_in_vol(...,ray_history(thread_id), thread_id);

The biggest pain here is going to be the number of default arguments that we typically don't use, see

ErrorCode ray_fire(const EntityHandle volume, const double ray_start[3],
                     const double ray_dir[3], EntityHandle& next_surf,
                     double& next_surf_dist,
                     RayHistory* history = NULL,
                     double dist_limit = 0, int ray_orientation = 1,
                     OrientedBoxTreeTool::TrvStats* stats = NULL);

But I do appreciate the simplicity of just passing in the thread id to function. This is one of the reasons I went the way I did, to avoid adding even more arguments to our already long list. What do you think?

pshriwise · 2018-09-18T21:11:21Z

I don't see the macros in this PR, but I know what you're talking about. I think they're a really good way to simplify those calls, but they'd either have to be redefined in every implementation we use or be exposed as part of the threading interface header somehow. At that point, it seems like they might as well be functions in the interface.

For the ray history, the default could be to look up the appropriate instance with the option of the user overriding that argument by passing NULL? Not totally happy with that, but I'd like to avoid extra calls in the physics codes if possible. The cleaner it looks there, the less perceived overhead DAGMC has and it keeps the focus on how to fit it into the transport process.

Re: default args - Yeah that would be a bit of a hassle, but not too bad, right? Just a copy of the signature and its defaults.

makeclean · 2018-09-18T21:16:24Z

My bad, I thought they were, I'll include them in the header, then the physics codes have access to them for free by bringing them in from the header. Actually, maybe you cant since you wont know the name for the thread manager pointer. I'll see what I can come up with.

makeclean · 2018-09-18T21:22:45Z

Hmm, when I run astyle I get 44 files reformatted, files that I haven't touched, im running astyle as in the travis test, but I have to remove the line continuation 80 line ( my package manager version doesnt support that one), does this sound right @ljacobson64?

ljacobson64 · 2018-09-21T01:40:04Z

@makeclean Unfortunately, different version of astyle give different results. Travis is using version 3.0.1. Can you try that one and see if the tests pass?

gonuke · 2018-12-13T22:10:13Z

This probably needs a rebase, but is it otherwise ready?

pshriwise · 2020-10-01T13:39:01Z

Is this still a work in progress @makeclean?

ahnaf-tahmid-chowdhury · 2024-02-29T14:02:16Z

Any update?

fresh rebase of threading for DAGMC

498dcc6

makeclean requested a review from pshriwise September 17, 2018 21:35

pshriwise mentioned this pull request Sep 18, 2018

Identify gaps in current technology preventing OpenMP support #4

Open

pshriwise reviewed Sep 18, 2018

View reviewed changes

makeclean added 3 commits September 18, 2018 21:04

C++03 ify the method to set the number of threads

ddf28ba

rename the thread manager

f80bc3a

update the cmakefile

be8d6d0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fresh rebase of threading for DAGMC #561

fresh rebase of threading for DAGMC #561

makeclean commented Sep 17, 2018

pshriwise commented Sep 17, 2018

pshriwise commented Sep 18, 2018

makeclean commented Sep 18, 2018

pshriwise left a comment

pshriwise Sep 18, 2018

pshriwise Sep 18, 2018

pshriwise Sep 18, 2018

makeclean Sep 18, 2018

pshriwise Sep 18, 2018

makeclean Sep 18, 2018

pshriwise Sep 18, 2018

makeclean Sep 18, 2018

pshriwise Sep 18, 2018

makeclean Sep 18, 2018

pshriwise Sep 18, 2018

makeclean Sep 18, 2018

pshriwise commented Sep 18, 2018

makeclean commented Sep 18, 2018

pshriwise commented Sep 18, 2018

makeclean commented Sep 18, 2018

pshriwise commented Sep 18, 2018

makeclean commented Sep 18, 2018 •

edited

pshriwise commented Sep 18, 2018 •

edited

makeclean commented Sep 18, 2018

makeclean commented Sep 18, 2018

ljacobson64 commented Sep 21, 2018

gonuke commented Dec 13, 2018

pshriwise commented Oct 1, 2020

ahnaf-tahmid-chowdhury commented Feb 29, 2024

		@@ -30,6 +29,10 @@ namespace moab {

		class CartVect;

		#define DAGMC_VERSION 3.0

fresh rebase of threading for DAGMC #561

Are you sure you want to change the base?

fresh rebase of threading for DAGMC #561

Conversation

makeclean commented Sep 17, 2018

pshriwise commented Sep 17, 2018

pshriwise commented Sep 18, 2018

makeclean commented Sep 18, 2018

pshriwise left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshriwise commented Sep 18, 2018

makeclean commented Sep 18, 2018

pshriwise commented Sep 18, 2018

makeclean commented Sep 18, 2018

pshriwise commented Sep 18, 2018

makeclean commented Sep 18, 2018 • edited

pshriwise commented Sep 18, 2018 • edited

makeclean commented Sep 18, 2018

makeclean commented Sep 18, 2018

ljacobson64 commented Sep 21, 2018

gonuke commented Dec 13, 2018

pshriwise commented Oct 1, 2020

ahnaf-tahmid-chowdhury commented Feb 29, 2024

makeclean commented Sep 18, 2018 •

edited

pshriwise commented Sep 18, 2018 •

edited