Clean headers from A-C #9365

brianlball · 2022-04-06T21:27:01Z

Clean Headers from A-C

clean header Boilers

amirroth · 2022-04-06T21:45:34Z

src/EnergyPlus/Boilers.hh

    bool getBoilerInputFlag = true;
    Array1D<Boilers::BoilerSpecs> Boiler;

    void clear_state() override
    {
-        this->numBoilers = 0;
-        this->getBoilerInputFlag = true;
-        this->Boiler.deallocate();


Why do we no longer have to call deallocate()? Did we not have to call it to begin with?

We don't need the deallocate anymore because we are creating a new instance. For the same reason, we don't need to manually go member-by-member resetting them to their original values anymore. It's so much nicer. Once we get all these clear_states cleaned out to the minimal new code, I think we will have a very rapid change where we don't really need the individual clear_state functions anymore. But we'll see...

So ... what is happening to the old instance? Does it get properly destroyed or are we leaking insane amounts of memory here? I admit I don't have a lot of experience with the *this = idiom. It looks very strange and wrong to me.

This should not be leaking anything. I made a prototype of this and called clear_state 50bn times and the memory usage on the process never went up, and I could see the destructor being called every time, so the assignment seems to be doing everything we need here. This paradigm was suggested by @lefticus, so I feel pretty good about it.

Now, that said, I think once we have all the clear_state functions cleaned up to this level, we can just go back to where we are calling clear_state, and change the call so that we don't need the intermediary clear_state function, if in fact all it is doing is deleting the last one and creating a new one.

(As a final aside, just note that this function is only called during serial unit test runs, and in case someone is calling clear_state during serial runs of E+ via API)

amirroth · 2022-04-06T21:53:44Z

src/EnergyPlus/ConvectionCoefficients.hh

-              MixedWindowsUserCurveNum(0)
-        {
-        }
+        int SimpleBuoyVertWallEqNum = ConvectionConstants::HcInt_FohannoPolidoriVerticalWall; // InConvClass_A3_VertWalls


Straight up one of the most ridiculous data structures in EnergyPlus.

amirroth · 2022-04-06T21:54:50Z

src/EnergyPlus/CurveManager.hh

+        Real64 CoeffA23 = 0.0;
+        Real64 CoeffA24 = 0.0;
+        Real64 CoeffA25 = 0.0;
+        Real64 CoeffA26 = 0.0;


What programming was like before arrays were invented.

amirroth · 2022-04-17T21:04:29Z

src/EnergyPlus/BoilerSteam.hh

-              PressErrIndex(0), FluidIndex(0), myFlag(true), myEnvrnFlag(true), FuelUsed(0.0), BoilerLoad(0.0), BoilerEff(0.0),
-              BoilerMassFlowRate(0.0), BoilerOutletTemp(0.0), BoilerEnergy(0.0), FuelConsumed(0.0), BoilerInletTemp(0.0),
-              BoilerFuelTypeForOutputVariable("")
+        BoilerSpecs() : plantLoc{}


Why is this necessary? Why is plantLoc default constructor not called automatically?

This is not necessary.

this was part of a diff debug and has been removed

amirroth · 2022-04-17T21:06:43Z

src/EnergyPlus/BoilerSteam.hh

@@ -171,15 +164,12 @@ namespace BoilerSteam {

 struct BoilerSteamData : BaseGlobalStruct
 {
-    int numBoilers = 0;
    bool getSteamBoilerInput = true;
    Array1D<BoilerSteam::BoilerSpecs> Boiler;


There is no reason for a 1D array of anything that isn't int, Real64, or bool to be an Array1D rather than an EPVector.

I agree, we are definitely moving in that direction. We can go back through these few files and add an extra effort to make sure all the Array1Ds are transitioned. We were trying to balance the header file cleanup with how many lines we had to change in the cc files. Changing this to a vector will cause quite a lot of changes, but I'm OK adding that here if you want.

amirroth · 2022-04-17T21:11:40Z

src/EnergyPlus/BranchInputManager.cc

-                state.dataBranchInputManager->Branch.allocate(state.dataBranchInputManager->NumOfBranches);
+            int NumOfBranches = state.dataInputProcessing->inputProcessor->getNumObjectsFound(state, CurrentModuleObject);
+            if (NumOfBranches > 0) {
+                state.dataBranchInputManager->Branch.allocate(NumOfBranches);
                for (auto &e : state.dataBranchInputManager->Branch)


Why is this loop necessary?

It doesn't look like it should be now. But it's been in there since the C++ translation. Back in Fortran, right after Branch was allocated, the AssignedLoopName was set to a blank. I think ....maybe.... it was because in Fortran you couldn't rely on the fixed-length strings to be filled with blank spaces? Maybe...?

amirroth · 2022-04-17T21:11:56Z

src/EnergyPlus/BranchInputManager.cc

-        state.dataBranchInputManager->NumOfBranchLists = state.dataInputProcessing->inputProcessor->getNumObjectsFound(state, CurrentModuleObject);
-        state.dataBranchInputManager->BranchList.allocate(state.dataBranchInputManager->NumOfBranchLists);
+        int NumOfBranchLists = state.dataInputProcessing->inputProcessor->getNumObjectsFound(state, CurrentModuleObject);
+        state.dataBranchInputManager->BranchList.allocate(NumOfBranchLists);
        for (auto &e : state.dataBranchInputManager->BranchList) {


Why is this loop necessary?

amirroth · 2022-04-17T21:18:29Z

src/EnergyPlus/BranchInputManager.cc

@@ -2743,7 +2743,7 @@ namespace BranchInputManager {
        }

        // Build node names in branches
-        for (Count = 1; Count <= state.dataBranchInputManager->NumOfBranches; ++Count) {
+        for (Count = 1; Count <= (int)state.dataBranchInputManager->Branch.size(); ++Count) {
            BranchNodes(Count).UniqueNodeNames.allocate(state.dataBranchInputManager->Branch(Count).NumOfComponents * 2);
            BranchNodes(Count).UniqueNodeNames = std::string();


You just did an allocate on this and now you assign it to a blank string because ... ?

I think these all go back to the original Fortran code, I'm not sure if there is a way we can automatically detect these unnecessary things now...but maybe. Anytime we reset a string to a blank seems like a good starting point, since that seems like an odd thing to do inside the simulation. Then we could add some intelligence to snoop just a couple lines before that assignment to see if it detects an allocation of that variable (or any variable?). Could be a fun excursion.

amirroth · 2022-04-17T21:25:08Z

src/EnergyPlus/ChilledCeilingPanelSimple.cc

-    state.dataChilledCeilingPanelSimple->CheckEquipName.allocate(state.dataChilledCeilingPanelSimple->NumCoolingPanels);
+    state.dataChilledCeilingPanelSimple->CoolingPanel.allocate(NumCoolingPanels);
+    state.dataChilledCeilingPanelSimple->CoolingPanelSysNumericFields.allocate(NumCoolingPanels);
+    state.dataChilledCeilingPanelSimple->CheckEquipName.allocate(NumCoolingPanels);
    state.dataChilledCeilingPanelSimple->CheckEquipName = true;


Does this assignment operator set all elements of the array to true? That is a terrible idiom. Is this a Fortran legacy thing?

Yes, I suppose, and yes, this is a Fortran legacy thing. Really, if the CheckEquipName needs to stay, it has no need to be its own separate array. It should be a simple boolean on the chilled ceiling struct. That's one better pattern we can propagate in this effort for sure.

Myoldmopar · 2022-04-21T19:33:01Z

GPU diff, otherwise CI is clean here. I think it's time to start pushing this toward merge. I need to digest the comments above next.

Myoldmopar

OK, so a net of 2400 lines removed. No diffs. Awesome. This is making a bunch of big improvements in the headers and state classes. I think it could use another pass based on my comments and some comments from @amirroth. And then we can take the outcomes of this branch and apply them to the next group of files shortly. Thanks @brianlball !

Myoldmopar · 2022-04-21T19:34:11Z

src/EnergyPlus/BoilerSteam.cc

@@ -161,10 +161,9 @@ namespace BoilerSteam {

        SteamFluidIndex = 0;
        state.dataIPShortCut->cCurrentModuleObject = "Boiler:Steam";
-        state.dataBoilerSteam->numBoilers =
-            state.dataInputProcessing->inputProcessor->getNumObjectsFound(state, state.dataIPShortCut->cCurrentModuleObject);
+        int numBoilers = state.dataInputProcessing->inputProcessor->getNumObjectsFound(state, state.dataIPShortCut->cCurrentModuleObject);


Yay, every time an unnecessary variable is removed from state and replaced with a local variable, an angel smiles.

Myoldmopar · 2022-04-21T19:35:08Z

src/EnergyPlus/BoilerSteam.cc

-            thisBoiler.FullLoadCoef(1) = state.dataIPShortCut->rNumericArgs(8);
-            thisBoiler.FullLoadCoef(2) = state.dataIPShortCut->rNumericArgs(9);
-            thisBoiler.FullLoadCoef(3) = state.dataIPShortCut->rNumericArgs(10);
+            thisBoiler.FullLoadCoef[0] = state.dataIPShortCut->rNumericArgs(8);


Ahhh, zero based indexing and square brackets, the way arrays were meant to be in C++ 😄

Myoldmopar · 2022-04-21T19:35:32Z

src/EnergyPlus/BoilerSteam.hh

-              PressErrIndex(0), FluidIndex(0), myFlag(true), myEnvrnFlag(true), FuelUsed(0.0), BoilerLoad(0.0), BoilerEff(0.0),
-              BoilerMassFlowRate(0.0), BoilerOutletTemp(0.0), BoilerEnergy(0.0), FuelConsumed(0.0), BoilerInletTemp(0.0),
-              BoilerFuelTypeForOutputVariable("")
+        BoilerSpecs() : plantLoc{}


This is not necessary.

Myoldmopar · 2022-04-21T19:37:49Z

src/EnergyPlus/BoilerSteam.hh

@@ -171,15 +164,12 @@ namespace BoilerSteam {

 struct BoilerSteamData : BaseGlobalStruct
 {
-    int numBoilers = 0;
    bool getSteamBoilerInput = true;
    Array1D<BoilerSteam::BoilerSpecs> Boiler;


I agree, we are definitely moving in that direction. We can go back through these few files and add an extra effort to make sure all the Array1Ds are transitioned. We were trying to balance the header file cleanup with how many lines we had to change in the cc files. Changing this to a vector will cause quite a lot of changes, but I'm OK adding that here if you want.

Myoldmopar · 2022-04-21T19:38:17Z

src/EnergyPlus/BoilerSteam.hh

-        this->numBoilers = 0;
-        this->getSteamBoilerInput = true;
-        this->Boiler.deallocate();
+        *this = BoilerSteamData();


Yay! So much DRYer!

Myoldmopar · 2022-04-22T14:02:50Z

src/EnergyPlus/CostEstimateManager.hh

-        this->DoCostEstimate = false;
-        this->numMonetaryUnit = 0;
-        this->selectedMonetaryUnit = 0;
-        this->CurntBldg = {0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0};


You didn't do anything wrong here, but I dislike that the default values on the struct are all 0.0, but in both cases where we instantiate them, we pass a 1.0 as one of the arguments. I think we should clean that up and just default assign it to 1.0 in the struct and then we don't have to have this initializer list anymore.

i think the default values have the same value as the reset now

Myoldmopar · 2022-04-22T14:03:17Z

src/EnergyPlus/CurveManager.hh

+        Real64 CoeffA23 = 0.0;
+        Real64 CoeffA24 = 0.0;
+        Real64 CoeffA25 = 0.0;
+        Real64 CoeffA26 = 0.0;


Myoldmopar · 2022-04-22T14:05:01Z

src/EnergyPlus/DataBranchNodeConnections.hh

-        this->NumNodeConnectionErrors = 0;
-        this->NumOfNodeConnections = 0;
-        this->MaxNumOfNodeConnections = 0;
-        this->NodeConnectionAlloc = 1000;


This variable smells funny.

It smells funny for two reasons:

We are carefully allocating out nodes during initialization in chunks, and I think we can rely on C++ to be smart enough to do this behind-the-scenes reservation. Anyway, the second reason is that

This variable is only used in one function, so I'm moving it to a constexpr int inside that function.

Myoldmopar · 2022-04-22T14:05:26Z

src/EnergyPlus/DataEnvironment.hh

-        this->varyingOrientationSchedIndex = 0;
-        this->forceBeginEnvResetSuppress = false;
-        this->oneTimeCompRptHeaderFlag = true;
+        *this = EnvironmentData();


Yay!!!!!!!!

Myoldmopar · 2022-04-22T14:05:40Z

src/EnergyPlus/DataGlobals.hh

-        this->printConsoleOutput = true;
-        this->installRootOverride = false;
-        this->numThread = 1;
+        *this = DataGlobal();


constructor cleanup

Myoldmopar · 2022-04-25T19:43:32Z

src/EnergyPlus/DataContaminantBalance.hh

@@ -288,8 +231,7 @@ struct ContaminantBalanceData : BaseGlobalStruct

    Array1D<Real64> CONTRAT;           // Zone CO2 at the previous time step used in Exact and Euler method
    Array1D<Real64> MixingMassFlowCO2; // Mixing MASS FLOW * CO2
-    int NumContControlledZones = 0;
-    Real64 OutdoorCO2 = 0.0; // Outdoor CO2 level


I'd love to go through this and clean up all the Array1D's, but I'm not going to add that to this PR.

Myoldmopar · 2022-04-25T19:45:33Z

src/EnergyPlus/DataLoopNode.hh

-        this->MoreNodeInfo.deallocate();
-        this->MarkedNode.deallocate();
-        this->NodeSetpointCheck.deallocate();
+        *this = LoopNodeData();


Myoldmopar · 2022-04-25T19:57:44Z

Just pushed a few commits that should get this ready to merge. This PR cleans up so much, assuming it's clean let's merge this in. If anyone has any remaining comments, speak up soon.

Myoldmopar · 2022-04-26T14:01:40Z

@amirroth thanks for commenting on this. I responded, and am happy to keep pushing on this as needed to resolve them, but if some of them are more just notes to 📌 for later, I'll just carry on to get this merged in.

Myoldmopar · 2022-04-26T15:49:45Z

Done, I think this should be ready to drop if clean then.

Myoldmopar · 2022-04-26T20:43:12Z

All green, merging this one in. Thanks @brianlball for the changes, and @amirroth for the commentary.

amirroth · 2022-04-26T21:03:40Z

Whatever the runtime implications of initializing in the object declaration, it's much more readable and understandable (given that you have the comments right there too) than having all of those in a giant constructor blob. Definitely a good development in terms of code readability and developer/reviewer sanity! Thanks!

brianlball added 22 commits April 5, 2022 11:12

clean header Boilers

79fbc9d

clean headers Boiler

25b274e

still need to call inputProcessor

8148d99

clean headers BoilerSteam

19bf1c3

clean header BranchInputManager

b973413

clean headers ChilledCeilingPanelSimple

a62ee19

clean header ChillerAbsorption

55d6bc5

clean header ChillerElectricEIR

f38370f

clean header ChillerExhaustAbsorption

2a37fd5

clean headers ChillerGasAbsorption

a98b3c5

clean headers ChillerIndirectAbsorption

6499051

clean headers ChillerReformulatedEIR

5ee62c4

clean headers CondenserLoopTowers

d92fe4a

clean headers Construction

0861fa1

clean headers ConvectionCoefficients

ea263b5

clean headers CoolTower

f9c36eb

clean header CostEstimateManager

42d35fb

clean headers CrossVentMgr

0582ec2

clean header CTElectricGenerator

a817e1b

clean headers CurveManager

104150b

Merge branch 'CleanHeadersFromAdown' into CleanHeadersFromAdown

d3eb5c6

Merge pull request #1 from NREL/CleanHeadersFromAdown

5a31ead

clean header Boilers

brianlball added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Apr 6, 2022

brianlball requested a review from Myoldmopar April 6, 2022 21:27

brianlball assigned Myoldmopar and brianlball and unassigned brianlball Apr 6, 2022

amirroth reviewed Apr 6, 2022

View reviewed changes

amirroth reviewed Apr 17, 2022

View reviewed changes

brianlball and others added 5 commits April 19, 2022 10:29

remove diff debugging

0e74ff2

Merge remote-tracking branch 'origin/develop' into CleanHeadersFromAdown

80424d1

Merge remote-tracking branch 'origin/develop' into CleanHeadersFromAdown

61802cd

Merge develop; resolve conflict

a292471

Clang format

ace3b3f

Myoldmopar reviewed Apr 22, 2022

View reviewed changes

brianlball and others added 3 commits April 25, 2022 11:03

Merge branch 'NREL:CleanHeadersFromAdown' into CleanHeadersFromAdown

20408ed

Merge pull request #9397 from brianlball/CleanHeadersFromAdown

4eb05d4

constructor cleanup

Clean out gobs of Array1D in ChilledCeiling

29504fe

Myoldmopar reviewed Apr 25, 2022

View reviewed changes

Myoldmopar added 3 commits April 25, 2022 14:53

Final polishing on this branch I think

55f4a5d

Merge remote-tracking branch 'origin/develop' into CleanHeadersFromAdown

b342e66

Clang format

7f22479

Change Set function to get function

50b207b

Clang format

6262e35

Myoldmopar merged commit ec07f58 into develop Apr 26, 2022

Myoldmopar deleted the CleanHeadersFromAdown branch April 26, 2022 20:43

Clean headers from A-C #9365

Clean headers from A-C #9365

Conversation

brianlball commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirroth Apr 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Apr 21, 2022

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Apr 25, 2022

Myoldmopar commented Apr 26, 2022

Myoldmopar commented Apr 26, 2022

Myoldmopar commented Apr 26, 2022

amirroth commented Apr 26, 2022

amirroth Apr 17, 2022 •

edited

Loading