-
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
Speed up nonsparse AD initial setup significantly #16089
Conversation
Approving a PR with 1/3 tests failing. I like that. Ballsy |
I live life on the edge. |
Job Documentation on ed82e42 wanted to post the following: View the site here This comment will be updated on new commits. |
@rwcarlsen what do you think about having |
Sigh...I'm actually pretty torn about what to do here. BISON for example initializes Maybe I should continue to have the This is of course another place where a sparse container is much less problematic than the nonsparse one. If I could take some input from @rwcarlsen @friedmud and @fdkong perhaps |
Also @dschwen |
Ideally, |
I totally agree. But it is hard to stop users from shooting themselves in the foot. For example look at what I have to do in 4c1a555. The developer who wrote |
Yea @dschwen wrote that code and we're not going to get any user-developers more wily than he. |
Can't you have the constructor of ADReal clear the derivative vector to zero even if |
Ok:
|
4c1a555
to
17e76a5
Compare
We have been setting `ADReal::do_derivatives=true` by default and only toggling to `false` during residual computation. This can be very bad for non-sparse calculations. For instance in a navier-stokes input file supplied by Gavin Ridley, I saw 10 seconds spent in initial condition computation and 9 seconds in `ComputeMaterialsObjectThread` for a total of 19 seconds in `FEProblemBase::initialSetup`. This was 27% of the total computation time! With the changes here `FEProblemBase::initialSetup` no longer even appears in the graph, which is how it should be. I recall that I was forced to do derivative calculations by default for the reasons stated in the comment I'm deleting. My memory says some phase field object was initializing its material properties in its constructor, or more likely during `initialSetup`, so we had to enable derivative calculations then or else its results would be garbage. Hopefully, however, if there are objects still doing things like that we can do a little more fine-grained control to make their objects work without significantly sabotaging everyone's performance. Refs idaholab#14701
17e76a5
to
ed82e42
Compare
That seems like a fair compromise of safety and speed. Let's see if changing that one |
@@ -1 +1 @@ | |||
Subproject commit 4f3fa5a6a2104ab8784a6519677589738b9aef6f | |||
Subproject commit 23a208e65851b46dba8ebb2822147187d8b7fa4b |
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.
Caution! This contains a submodule update
Now I'm just getting a crap-ton of valgrind errors out of
|
I could not have less clue looking through the function parser code where |
|
Valgrind getting fooled by optmizations. In debug mode, I get the right stack traces:
Default constructor for template <typename T>
inline
TypeVector<T>::TypeVector ()
{
_coords[0] = {};
#if LIBMESH_DIM > 1
_coords[1] = {};
#endif
#if LIBMESH_DIM > 2
_coords[2] = {};
#endif
} So this would require applying the same patch to |
I help clean those up if you point them out, they're probably mine... |
@lindsayad - objects with thread copies are the only places that don't need to use the lock guard. I'd maybe try making a function like this - basically trying to encapsulate the mutex and logic required for taking the lock, etc: void FEProblemBase::withDerivativesAs(bool do_derivatives, std::function<()> func)
{
std::lock_guard<std::mutex> guard(_do_derivatives_mutex);
func();
} Then basically use this function everywhere - explicitly doing operations with it set a particular way. But I haven't really dug into all the code and details here - so I'll trust what you end up doing makes the most sense. |
We have been setting
ADReal::do_derivatives=true
by default and onlytoggling to
false
during residual computation. This can be very badfor non-sparse calculations. For instance in a navier-stokes input file
supplied by @gridley, I saw 10 seconds spent in initial condition
computation and 9 seconds in
ComputeMaterialsObjectThread
for a totalof 19 seconds in
FEProblemBase::initialSetup
. This was 27% of thetotal computation time! With the changes here
FEProblemBase::initialSetup
no longer even appears in the graph, whichis how it should be.
Graph with old toggling
Graph with new toggling
I recall that I was forced to do derivative calculations by default for
the reasons stated in the comment I'm deleting. My memory says some
phase field object was initializing its material properties in its
constructor, or more likely during
initialSetup
, so we had to enablederivative calculations then or else its results would be garbage.
Hopefully, however, if there are objects still doing things like that we
can do a little more fine-grained control to make their objects work
without significantly sabotaging everyone's performance.
Refs #14701