Threaded General User Objects #11834

andrsd · 2018-07-11T21:56:23Z

Allow creating threaded copies of general user objects

andrsd · 2018-07-11T21:58:51Z

@jasondhales Can you test this branch with your BISON changes to see if that fixes the threading problem you described in #11731? Thanks!

jasondhales · 2018-07-11T22:59:34Z

If I change "threaded_copies" to "threaded" in FluidProperties.C, it works.

andrsd · 2018-07-12T13:52:34Z

Thanks for catching that!

moosebuild · 2018-07-12T14:29:04Z

Job Documentation on 94476f4 wanted to post the following:

View the site here

This comment will be updated on new commits.

friedmud

Some issues with execution

friedmud · 2018-07-12T15:07:59Z

framework/src/loops/ComputeThreadedGeneralUserObjectsThread.C

+    ParallelUniqueId puid;
+    _tid = bypass_threading ? 0 : puid.id;
+
+    const auto & objects = _user_objects.getActiveObjects();


I don't get it - won't this make every thread execute every object?

friedmud · 2018-07-12T15:09:12Z

framework/src/problems/FEProblemBase.C

+  if (threaded_general.hasActiveObjects())
+  {
+    ComputeThreadedGeneralUserObjectsThread ctguot(*this, threaded_general);
+    Threads::parallel_reduce(EmptyRange(), ctguot);


This is what I was saying we shouldn't do. You need to actually pass the range of userobjects here to ensure they all get executed.

moosebuild · 2018-07-13T14:53:49Z

Job App tests on d732de0 : invalidated by @andrsd

grizzly is up-to-date

rwcarlsen · 2018-07-16T19:14:45Z

This makes me sad - I will have to rewrite this code and resolve conflicts :-/

jasondhales · 2018-07-17T20:32:36Z

I'm not sure why this is an issue for @rwcarlsen, but I'm eager to see this merged.

friedmud

The code here looks good - but I would like to see one test of it. Can you dream up something that would test this?

andrsd · 2018-07-18T20:44:23Z

The code here looks good - but I would like to see one test of it. Can you dream up something that would test this?

I flipped the "threaded" flag for the FluidProperty class, so this would be tested whenever we run tests in the fluid_property module. If that is not enough, I could build an user object that has some state, so that when it runs threaded and does not have the threaded flag on, it would produce a wrong result. If we get "lucky". Other than that, I am not sure how else to test this.

rwcarlsen · 2018-07-18T21:32:23Z

It would be helpful if we have a reliable test (that fails without this fix/feature) - if we don't I'm likely to introduce regressions soon when I rework the code.

…

On Wed, Jul 18, 2018 at 2:44 PM, David Andrs ***@***.***> wrote: The code here looks good - but I would like to see one test of it. Can you dream up something that would test this? I flipped the "threaded" flag for the FluidProperty class, so this would be tested whenever we run tests in the fluid_property module. If that is not enough, I could build an user object that has some state, so that when it runs threaded and does not have the threaded flag on, it would produce a wrong result. If we get "lucky". Other than that, I am not sure how else to test this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11834 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABSeSqhK2YuGVjJdBata72T5ip-Pm-k1ks5uH54pgaJpZM4VL6Je> .

andrsd · 2018-07-18T21:38:50Z

The problem is that the result would depend on the thread scheduler which we have no control over, so the test could sometimes pass even if there was something wrong :-(

friedmud · 2018-07-18T21:43:34Z

The thread scheduler doesn't matter here - every threaded object will get executed... even if not on separate threads. All we need to verify is that each one gets executed... And, yes, I would like a reliable test in moose_test before this can go in. It's a tricky enough capability that we need to make sure it stays working correctly.

…

On Wed, Jul 18, 2018 at 3:38 PM David Andrs ***@***.***> wrote: The problem is that the result would depend on the thread scheduler which we have no control over, so the test could sometimes pass even if there was something wrong :-( — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#11834 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA1JMfo1FVlhePg6xYhsMWv9QkRonJWaks5uH6rqgaJpZM4VL6Je> .

andrsd · 2018-07-19T18:12:33Z

(Hopefully) reliable test added...

permcody · 2018-07-27T14:03:57Z

ping @friedmud? 8 days no action

friedmud

Issue with interdependencies between these objects.

friedmud · 2018-07-30T14:51:48Z

framework/src/loops/ComputeThreadedGeneralUserObjectsThread.C

+{
+  try
+  {
+    for (auto it = range.begin(); it != range.end(); ++it)


friedmud · 2018-07-30T15:06:51Z

framework/src/problems/FEProblemBase.C

+      thread_ids[tid] = tid;
+
+    ComputeThreadedGeneralUserObjectsThread ctguot(*this, threaded_general);
+    Threads::parallel_reduce(ThreadIdRange(thread_ids.begin(), thread_ids.end()), ctguot);


Ok - the new comment is that this doesn't work with interdependencies. Each ThreadedGeneralUserObject needs its own thread loop - then call threadjoin() and finalize() on it before starting the next one.

Should be all in CAPS Refs #000

moosebuild · 2018-08-07T19:22:14Z

Job Precheck on cd5f209 wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s http://mooseframework.inl.gov/docs/PRs/11834/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format a9718a59f043e1a2de62c5c7bde1185ce3f0562c

Now, general users objects can be build per thread. User can set "threaded" flag to inform MOOSE thay require threaded copies of their user objects. Closes idaholab#11731

To make them thread safe when called from Materials, etc. Refs idaholab#11731

Need to get params from GeneralUserObject not just UserObject.

Have to get validParams from your base class. Refs #000

Have to get validParams from a base class. Refs #000

If a general user object used threads inside, we would have a threaded region inside a threaded region and that would destroy people's machine. We want to prevent that, so we only allow threaded general user objects with OpenMP and TBB.

friedmud

👍

brianmoose · 2018-08-08T15:35:21Z

Looks like our minimum clang testing has a problem with this PR: https://civet.inl.gov/job/218243/

andrsd · 2018-08-08T16:31:40Z

#11961 should fix that

Since a threaded (vs not) object requires very specific code handling, we do not generally want the ability for objects to be dynamically toggled between threaded vs not handling - this could easily result in unintentional, subtle errors. Also, the new general warehouse handling of user objects will be much better served by this as an explicit separate class. ref idaholab#11834

Since a threaded (vs not) object requires very specific code handling, we do not generally want the ability for objects to be dynamically toggled between threaded vs not handling - this could easily result in unintentional, subtle errors. Also, the new general warehouse handling of user objects will be much better served by this as an explicit, separate class. ref idaholab#11834

When ThreadedGeneraluserObject is requested via UserObjectInterface, we respect the thread ID and give users back the threaded copy of the user object. Refs idaholab#11834

When ThreadedGeneraluserObject is requested via UserObjectInterface, we respect the thread ID and give users back the corresponding thread copy. Refs idaholab#11834

When ThreadedGeneralUserObject is requested via UserObjectInterface, we respect the thread ID and give users back the corresponding thread copy. Refs idaholab#11834

andrsd force-pushed the 11731 branch from b57a662 to 01115f6 Compare July 11, 2018 21:56

andrsd force-pushed the 11731 branch from 01115f6 to 21001fc Compare July 12, 2018 13:48

andrsd force-pushed the 11731 branch from 21001fc to c103549 Compare July 12, 2018 13:57

friedmud requested changes Jul 12, 2018

View reviewed changes

andrsd force-pushed the 11731 branch from 983007c to d732de0 Compare July 12, 2018 18:13

permcody assigned friedmud Jul 13, 2018

friedmud requested changes Jul 18, 2018

View reviewed changes

andrsd force-pushed the 11731 branch 2 times, most recently from 2986d2e to 64e35d6 Compare July 19, 2018 18:11

friedmud requested changes Jul 30, 2018

View reviewed changes

friedmud added the PR: Address comments label Jul 30, 2018

Fixing #include guard

625ea27

Should be all in CAPS Refs #000

andrsd force-pushed the 11731 branch from 64e35d6 to c50d6da Compare August 7, 2018 19:19

moosebuild removed the PR: Address comments label Aug 7, 2018

andrsd force-pushed the 11731 branch from c50d6da to cd5f209 Compare August 7, 2018 19:20

Threaded general user objects

080a177

Now, general users objects can be build per thread. User can set "threaded" flag to inform MOOSE thay require threaded copies of their user objects. Closes idaholab#11731

andrsd added 6 commits August 7, 2018 14:02

Create threaded copies of FluidProperties objects

f969e3a

To make them thread safe when called from Materials, etc. Refs idaholab#11731

Fixing validParams in ReadDoubleIndex class

c20b945

Need to get params from GeneralUserObject not just UserObject.

Fixing validParams in CrackDataSampler

20416d9

Have to get validParams from your base class. Refs #000

Fixing validParams in GrainForcesPostprocessor

2f66a22

Have to get validParams from a base class. Refs #000

Adding tests

94476f4

andrsd force-pushed the 11731 branch from cd5f209 to 94476f4 Compare August 7, 2018 20:03

friedmud approved these changes Aug 8, 2018

View reviewed changes

friedmud merged commit 13a81b9 into idaholab:devel Aug 8, 2018

andrsd deleted the 11731 branch August 8, 2018 16:30

rwcarlsen mentioned this pull request Aug 10, 2018

give threaded general user objects their own subclass #11979

Merged

brianmoose mentioned this pull request Aug 13, 2018

Don't run new porous_flow injection_production test with pthreads #11977

Merged

andrsd mentioned this pull request Aug 16, 2018

Get the right thread copy when requesting ThreadedGeneralUserObject #12008

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Threaded General User Objects #11834

Threaded General User Objects #11834

andrsd commented Jul 11, 2018

andrsd commented Jul 11, 2018

jasondhales commented Jul 11, 2018

andrsd commented Jul 12, 2018

moosebuild commented Jul 12, 2018 •

edited

friedmud left a comment

friedmud Jul 12, 2018

friedmud Jul 12, 2018

moosebuild commented Jul 13, 2018

rwcarlsen commented Jul 16, 2018 •

edited

jasondhales commented Jul 17, 2018

friedmud left a comment

andrsd commented Jul 18, 2018

rwcarlsen commented Jul 18, 2018 via email

andrsd commented Jul 18, 2018

friedmud commented Jul 18, 2018 via email

andrsd commented Jul 19, 2018

permcody commented Jul 27, 2018

friedmud left a comment

friedmud Jul 30, 2018

friedmud Jul 30, 2018

moosebuild commented Aug 7, 2018

friedmud left a comment

brianmoose commented Aug 8, 2018

andrsd commented Aug 8, 2018

Threaded General User Objects #11834

Threaded General User Objects #11834

Conversation

andrsd commented Jul 11, 2018

andrsd commented Jul 11, 2018

jasondhales commented Jul 11, 2018

andrsd commented Jul 12, 2018

moosebuild commented Jul 12, 2018 • edited

friedmud left a comment

Choose a reason for hiding this comment

friedmud Jul 12, 2018

Choose a reason for hiding this comment

friedmud Jul 12, 2018

Choose a reason for hiding this comment

moosebuild commented Jul 13, 2018

rwcarlsen commented Jul 16, 2018 • edited

jasondhales commented Jul 17, 2018

friedmud left a comment

Choose a reason for hiding this comment

andrsd commented Jul 18, 2018

rwcarlsen commented Jul 18, 2018 via email

andrsd commented Jul 18, 2018

friedmud commented Jul 18, 2018 via email

andrsd commented Jul 19, 2018

permcody commented Jul 27, 2018

friedmud left a comment

Choose a reason for hiding this comment

friedmud Jul 30, 2018

Choose a reason for hiding this comment

friedmud Jul 30, 2018

Choose a reason for hiding this comment

moosebuild commented Aug 7, 2018

friedmud left a comment

Choose a reason for hiding this comment

brianmoose commented Aug 8, 2018

andrsd commented Aug 8, 2018

moosebuild commented Jul 12, 2018 •

edited

rwcarlsen commented Jul 16, 2018 •

edited