-
Notifications
You must be signed in to change notification settings - Fork 21
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
Transition to DA build only #77
Comments
I prefer it only because that is how it was built in our CVS GNUmake build that we are moving to Git. And my mandate was to make sure all the flags were exactly the same (so we could get the same answer). My guess is the new BUFRLIB was brought in and "things" were done until it worked (this was after I moved from maintaining the CVS/GNU build to our current Git/CMake). Maybe they didn't know about the dynamic allocation build? I'll propose we move to it, but I don't know how to evaluate if doing so changes results. Note we only saw this because (until now) our main GCM didn't build this library as it didn't need it. But recent work has discovered it will be needed, so it was added and our CI system (which is all GCC) threw a rod. |
@edwardhartnett I have a reply from one of our devs which I'll quote here:
Now the I took a look and it looks like in your latest release (11.3.2) and in I guess we will try and look at the new BUFR code soon and evaluate it. |
Thanks @mathomp4 ! |
I was about to open a new issue to enable testing for all of the static and dynamic builds when I came across this issue which is still open. @edwardhartnett can you tell me what the original rationale was for opening this ticket to eliminate support for static builds? I'm not saying this couldn't be done at some point, but it's not something we should do without a lot of prior coordination, especially for WCOSS users where it's always a constant battle to improve the timeliness and efficiency of program runs for NWP, and where using dynamic allocation obviously leads to at least a slight increase in run times. |
Just to clarify, and for backreferencing purposes, I was about to open a new issue (to enable testing for all static and dynamic builds) as a result of the discussion in #94 , before I noticed that this issue was still open ;-) |
If I understand correctly, by "static builds" you mean a special build
which does not use malloc. This is different from the usual meaning of
static builds, which means the .a build (as opposed to the .so shared
library build). For this reason I will call this the "static memory" build,
to differentiate from the "static library" build.
I would like to see proof of increase in run-time due to static memory
build. I strongly suspect that this is an unproven and unnecessary
optimization.
No other library in our stack does this. NetCDF, HDF5, eccodes, etc., all
use malloc freely with no apparent loss of performance. Actually, I have
never encountered any other package that tries a static memory build. Since
the model run is limited by computation and I/O, it seems highly unlikely
that any reasonable use of malloc would make a difference in run time.
Mallocs and frees are not particularly expensive operations (
https://stackoverflow.com/questions/7612292/how-bad-it-is-to-keep-calling-malloc-and-free),
in fact they are fast.
Many of the NCEPLIBS do things that no one else does. Nothing is free. If
these things do not bring benefit, we must stop doing them and focus our
efforts on work that does bring benefit.
Everything that we do differently from other teams needs to be carefully
examined. It's extremely unlikely that we have discovered a good form of
optimization that everyone else has missed. If we have, we need to prove
it, and spread the knowledge. If we haven't, we need to do things in an
industry-standard way, to control costs.
This does not affect whether you set up testing for both static and dynamic
memory builds. We need to test both as long as we support both, and
currently we are supporting both so need to test both. When the static
memory build is discontinued, we will then stop testing it. Since, as you
say, we have to work with users to transition the to the dynamic memory
build, we cannot stop supporting the static memory build today.
Double the testing is one example of how this is not free.
…On Tue, Jan 12, 2021 at 9:45 AM Jeff Ator ***@***.***> wrote:
I was about to open a new issue to enable testing for all of the static
and dynamic builds when I came across this issue which is still open.
@edwardhartnett <https://github.com/edwardhartnett> can you tell me what
the original rationale was for opening this ticket to eliminate support for
static builds? I'm not saying this couldn't be done at some point, but it's
not something we should do without a lot of prior coordination, especially
for WCOSS users where it's always a constant battle to improve the
timeliness and efficiency of program runs for NWP, and where using dynamic
allocation obviously leads to at least a slight increase in run times.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#77 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJIOMME5EWHLB6N4D5D7PN3SZR4AVANCNFSM4TYNEEBA>
.
|
Yes, I'm talking about "static memory" builds and not "static library" builds. I realize those are two different things, as I noted in the discussion of #94 I wasn't aware that static allocation of memory was considered such a non-standard way of doing things, nor that studies have been done showing that dynamic allocation of memory isn't particularly expensive. If that's the case, then again I'm fine with a gradual move towards discontinuing support for the static memory builds, maybe not in time for the next 11.4.0 release but at least for the subsequent one thereafter. So until then we'll just keep this issue open, and I agree that until then we should also resume testing of static memory builds on all platforms. |
Currently twe build a static and a dynamic allocation memory model.
Let's eliminate the static build and make the dynamic build the only build.
The text was updated successfully, but these errors were encountered: