Skip to content
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

VectorPostprocessor threads design #7809

Closed
permcody opened this issue Oct 4, 2016 · 3 comments
Closed

VectorPostprocessor threads design #7809

permcody opened this issue Oct 4, 2016 · 3 comments

Comments

@permcody
Copy link
Member

permcody commented Oct 4, 2016

I'm hoping to create a discussion on this ticket rather than claiming there's a design issue somewhere. @dschwen just created a VectorPostprocessor where he was getting exponential growth on his vectors based on the number of threads he was using. After a little digging we discovered that the problem was that VectorPostprocessor references come from the MOOSE data store and we only have one copy among all threads. This seems problematic...

I bring this up because vector postprocessors are supposed to be a utility created by developers and we've even defined the same interface as the Postprocessor class with threadJoin and finalize methods. The current design definitely lends itself to a design trap when implementing a new threaded (Element or Nodal) vector postprocessor. I think most developers would assume that accessing and writing to the vector would be just fine in the execute method when it is in fact we can't expect good behavior from that operation.

I think we should chat about ways to avoid this, ways to redesign the current vector data store, or ways to train users that this is unsafe behavior. Ideas are welcome.

tagging @idaholab/moose-team

@friedmud
Copy link
Contributor

friedmud commented Oct 4, 2016

Not sure that there is a general design that can cover all possibilities
here. I would say that in general you should hold off on writing to the
actual output vectors until finalize()... that should be pretty safe
guidance... but it may not always be possible.

On Tue, Oct 4, 2016 at 4:35 PM Cody Permann notifications@github.com
wrote:

I'm hoping to create a discussion on this ticket rather than claiming
there's a design issue somewhere. @dschwen https://github.com/dschwen
just created a VectorPostprocessor where he was getting exponential growth
on his vectors based on the number of threads he was using. After a little
digging we discovered that the problem was that VectorPostprocessor
references come from the MOOSE data store and we only have one copy among
all threads. This seems problematic...

I bring this up because vector postprocessors are supposed to be a utility
created by developers and we've even defined the same interface as the
Postprocessor class with threadJoin and finalize methods. The current
design definitely lends itself to a design trap when implementing a new
threaded (Element or Nodal) vector postprocessor. I think most developers
would assume that accessing and writing to the vector would be just fine in
the execute method when it is in fact we can't expect good behavior from
that operation.

I think we should chat about ways to avoid this, ways to redesign the
current vector data store, or ways to train users that this is unsafe
behavior. Ideas are welcome.

tagging @idaholab/moose-team
https://github.com/orgs/idaholab/teams/moose-team


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#7809, or mute the thread
https://github.com/notifications/unsubscribe-auth/AA1JMbu9VsQZ1pVViSd2CetJ6VEUVScoks5qwriXgaJpZM4KOIJu
.

@permcody
Copy link
Member Author

permcody commented Oct 5, 2016

Yeah, the problem is that this logic error came out of implementing a threadJoin() due to a lack of critical thinking or understanding of the VectorPostprocessorValue shared storage variable among an otherwise list of thread-local storage variables. It's just fundamentally different from all other pluggable systems in MOOSE because to the best of my knowledge it's the only place where we have to write to shared storage instead of just reading from it.

Since declarVector() is a method on the VectorPostprocessor object, we could use a threadID internally to hand back TLS references to individual vectors. Then it would "feel" more like working with other similar objects. The advice of using threadJoin() to aggregate values would be the same between Postprocessor, UserObject, and VectorPostprocessor

permcody added a commit to permcody/moose that referenced this issue Oct 5, 2016
After this is merged we can go through the existing samplers and remove temporary vectors
or other workarounds to not having TLS.

refs idaholab#7809
permcody added a commit to permcody/moose that referenced this issue Oct 7, 2016
After this is merged we can go through the existing samplers and remove temporary vectors
or other workarounds to not having TLS.

refs idaholab#7809
permcody added a commit to permcody/moose that referenced this issue Oct 12, 2016
After this is merged we can go through the existing samplers and remove temporary vectors
or other workarounds to not having TLS.

refs idaholab#7809
permcody added a commit to permcody/moose that referenced this issue Oct 12, 2016
After this is merged we can go through the existing samplers and remove temporary vectors
or other workarounds to not having TLS.

refs idaholab#7809
@permcody
Copy link
Member Author

permcody commented Jan 9, 2017

This issue was closed with the thread local storage area being set up so that threads may concurrently write to a vector without issue (#7819).

@permcody permcody closed this as completed Jan 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants