-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Threaded General User Objects #11834
Conversation
@jasondhales Can you test this branch with your BISON changes to see if that fixes the threading problem you described in #11731? Thanks! |
If I change "threaded_copies" to "threaded" in FluidProperties.C, it works. |
Thanks for catching that! |
Job Documentation on 94476f4 wanted to post the following: View the site here This comment will be updated on new commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some issues with execution
ParallelUniqueId puid; | ||
_tid = bypass_threading ? 0 : puid.id; | ||
|
||
const auto & objects = _user_objects.getActiveObjects(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get it - won't this make every thread execute every object?
if (threaded_general.hasActiveObjects()) | ||
{ | ||
ComputeThreadedGeneralUserObjectsThread ctguot(*this, threaded_general); | ||
Threads::parallel_reduce(EmptyRange(), ctguot); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This makes me sad - I will have to rewrite this code and resolve conflicts :-/ |
I'm not sure why this is an issue for @rwcarlsen, but I'm eager to see this merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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>
.
|
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 :-( |
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>
.
|
2986d2e
to
64e35d6
Compare
(Hopefully) reliable test added... |
ping @friedmud? 8 days no action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue with interdependencies between these objects.
{ | ||
try | ||
{ | ||
for (auto it = range.begin(); it != range.end(); ++it) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
thread_ids[tid] = tid; | ||
|
||
ComputeThreadedGeneralUserObjectsThread ctguot(*this, threaded_general); | ||
Threads::parallel_reduce(ThreadIdRange(thread_ids.begin(), thread_ids.end()), ctguot); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Job Precheck on cd5f209 wanted to post the following: Your code requires style changes. A patch was auto generated and copied here
Alternatively, with your repository up to date and in the top level of your repository:
|
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Looks like our minimum clang testing has a problem with this PR: https://civet.inl.gov/job/218243/ |
#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
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
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
When ThreadedGeneralUserObject is requested via UserObjectInterface, we respect the thread ID and give users back the corresponding thread copy. Refs idaholab#11834
Allow creating threaded copies of general user objects