-
Notifications
You must be signed in to change notification settings - Fork 390
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
EnergyPlus Build Time Increasing #8598
Comments
Research and prototyping of possibilities is commencing on this branch: https://github.com/NREL/EnergyPlus/tree/prototype_simplified_array1d A full description of the initial findings is available on this commit: |
Could this be related to the state transition? I don’t see why it would be, but that is a big change that has been happening recently and incrementally. |
It is related, yes. It's partly due to increased visibility of See my analysis on the this commit b036efa |
I see. Thanks. I am also a big proponent of thinning out whichever ObjexxFCL array libraries we end up keeping. There are a bunch of classes and operators that need to be in there for FORTRAN coverage purposes, but that we don't actually want to use in EnergyPlus, like array arithmetic, initialization (we use this a lot but should move away from it), and slicing (we use this less, but we need to stop). I'm not sure that these are great language features and I am pretty sure that they are terrible template library features. |
I fully agree that it does not make sense to remove Objexx's multi-dimensional arrays where we use them and their math functions. it would be far too much work to move to something different and pull in a new dependency. Here is a proposal for decreasing build times while moving the E+ code base to more standard C++ (eg, 0-based indexing) where it makes sense to.
Note: reduction of ObjexxFCL and Array1D in particular should have a significant impact on binary sizes. It is hopeful that binary size reduction will also increase performance due to less cache pressure Further reduction or removal of slices are up to the primary maintainers and developers of EnergyPlus, I don't know what impact this would have on the code, either from a compile-time perspective or a maintainability perspective. Note 2: The goal is not to eliminate or even reduce any of the uses of |
|
Since it seems to have been forgotten in this context, I'll remind everyone that there was to be a cooperative effort on the Array1D replacement front that seems to have largely fallen through. Which is a shame, because we even had arrays of bools working. Anyway, PRs #7813 and #7841 cleaned up a lot of the unnecessary uses of the fancier array objects from ObjexxFCL. Some of the remainders are in code that we would have had to really understand to do a diff-free replacement, so I'm a little skeptical about a week being enough time. I know I'll regret engaging on this, but once more into the breach. I believe it'd be more accurate to say that EnergyPlus was written in Fortran by FORTRAN programmers with many parts based on older FORTRAN code.
|
@jasondegraw agreed re: I was not aware of the existing PR's, but they could be very helpful here, I was just tasked with improving build times and found that Array1D is one of our main problems. I do, of course, prefer @amirroth I have no strongly held opinions on slices, because I do not know how they are used in the EnergyPlus code. My one interaction with slices in EnergyPlus was a 4D slice view being created from a 5D Array, which appeared to be a non-trivial use of it. But, there can be very good performance and API reasons to using slice-like things (C++20's std::span, for example, which is very easy to implement in C++17), because they give you nice ranged access to contiguous data which is decoupled from the underlying data structure, without needing to templatize the function being called. This could be very helpful for easily allowing common functions to be called with a |
Jason. Jason. Should we invite Jason Glazer to this thread? Sorry, I don't know what I was thinking about I guess the current use of slicing in EnergyPlus appears to be minimal because @jasondegraw has gotten rid of many of the original uses. Thanks @jasondegraw! (I didn't mean to discount your contributions, I just lost track of them). Several of the remaining instances I have run into (actually @xuanluo113 ran into them) can be eliminated by transposing the indices of the parent matrix (there are some multi-dimensional arrays that still assume column-major ordering apparently) and replacing the transposed 2D array by a 1D array of 1D arrays. This would also save some "virtual space" because only a small fraction of the inner 1D arrays would need to be created. A few other instances could be handled by the I apologize for the blanket slander of all EnergyPlus and predecessor FORTRAN developers. I will be sure to redact all names from the "Crimes against EnergyPlus" card series I am developing. |
@lefticus In the last PR I referenced we had gotten to the point that we were running up against multidimensional arrays as the major impediment to progress. Our stand-in for Array1D (EPVector in EnergyPlus.hh) had grown to have a few methods we didn't really want, but at least in a few cases we were able to add a method to get things compiling, then go back and do the correct thing, and finally remove the method. We had some progress spreadsheets someplace, I'll try to find those. @amirroth I hope to be featured prominently as "that airflow guy". More seriously though, FORTRAN77 and previous had a limited set of data structures available, and some of us were taught to avoid things too far from common usage. The variations between compiler could be pretty extreme, but arrays would always be there, so... yeah... arrays. The plain vanilla F77 CFD code that I inherited for use on my masters still compiles and works and it dates to the 1980s, before the invention of lower case. My recollection is that there were places where "arrays of arrays" would slot right in and eliminate more |
Issue overview
The build time for EnergyPlus has steadily increased as of late. It's not a crazy amount, but it is becoming annoying, and starting to impact our CI's ability to produce tests in reasonable time.
The text was updated successfully, but these errors were encountered: