SurfaceWindow struct refactoring #8141

xuanluo113 · 2020-07-08T21:57:10Z

Pull request overview

The PR includes several refactoring effort on the SurfaceWindow related data structures, including:

Convert the SurfaceWindow object arrays to a list of scalar arrays (src/EnergyPlus/DataSurfaces.cc). This improves cache locality.
Reduce the scope of several Surface loops to ZoneSurface loops, nested with a SurfOpaq loop and a SurfWin loop separately, moving the branches out of the loop to help vectorization.
Rename the surface window scalar arrays in src/EnergyPlus/HeatBalanceSurfaceManager.cc by adding a prefix - "SurfWin" to be consistent with the SurfaceWindow naming conventions. This is to benefit follow-up efforts on continuing to reduce the scope of window-only local arrays, loops, and functionalities.
Convert the timestep reinitialization of Surface and SurfWin arrays from the array-wise assignment (Array1D A = 0.0) to the element-wise assignment. This helps compiler level optimization.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

Reviewer

This will not be exhaustively relevant to every PR.

…nto window-test

…dow-test

xuanluo113 · 2020-09-01T17:23:52Z

@mjwitte @Myoldmopar This branch is ready for review and the refactoring efforts included in the current commits are listed in the PR overview. The CI is clean and CI performance suite is showing a 1.89% speedup.
Some other surface arrays refactoring efforts are ongoing, but merging is getting harder so I clean up some solid features and hope to get them reviewed and merged first. I'll start another branch/draft PR to work on it and keep this one as it is. Please let me know if you suggest otherwise.
@mjwitte If you want to start your investigation on movable insulation on top of this, please refer to this branch.

amirroth · 2020-09-01T17:35:08Z

Xuan. I see that I am listed as a reviewer. I assume that is just a courtesy. :)

…

On Tue, Sep 1, 2020 at 1:24 PM Xuan Luo ***@***.***> wrote: @mjwitte <https://github.com/mjwitte> @Myoldmopar <https://github.com/Myoldmopar> This branch is ready for review and the refactoring efforts included in the current commits are listed in the PR overview. The CI is clean and CI performance suite is showing a 1.89% speedup. Some other surface arrays refactoring efforts are ongoing, but merging is getting harder so I clean up some solid features and hope to get them reviewed and merged first. I'll start another branch/draft PR to work on it and keep this one as it is. Please let me know if you suggest otherwise. @mjwitte <https://github.com/mjwitte> If you want to start your investigation on movable insulation on top of this, please refer to this branch. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#8141 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABWYSISI7GHVZEJEBENQZFTSDUU3PANCNFSM4OU7UDPA> .

--

__________________________________________ Amir Roth, PhD http://energy.gov/eere/buildings/building-energy-modeling/

Myoldmopar

I'll build locally with develop one more time and run tests.

Myoldmopar · 2020-09-02T20:39:19Z

src/EnergyPlus/AirflowNetworkBalanceManager.cc

@@ -1548,6 +1547,7 @@ namespace AirflowNetworkBalanceManager {
        using DataLoopNode::NodeConnectionType_Inlet;
        using DataLoopNode::NodeType_Air;
        using DataLoopNode::ObjectIsParent;
+        using DataSurfaces::SurfWinOriginalClass;


Adding more usings isn't great. Probably not worth more commits, but we are moving away from it. Even if it makes the lines of code a little wider...that's hardly a problem on most folks monitors these days.

Myoldmopar · 2020-09-02T20:39:34Z

src/EnergyPlus/AirflowNetworkBalanceManager.cc

@@ -5153,19 +5153,19 @@ namespace AirflowNetworkBalanceManager {
                }
                SetupOutputVariable("AFN Surface Venting Window or Door Opening Modulation Multiplier",
                                    OutputProcessor::Unit::None,
-                                    SurfaceWindow(SurfNum).VentingOpenFactorMultRep,
+                                    DataSurfaces::SurfWinVentingOpenFactorMultRep(SurfNum),


And in some cases, you put the namespace to avoid the using.

Myoldmopar · 2020-09-02T20:42:01Z

src/EnergyPlus/InternalHeatGains.cc

-                // store the magnitude and time of the pulse
-                radiantPulseTimestep(CurOverallSimDay, NZ) = (HourOfDay - 1) * NumOfTimeStepInHour + TimeStep;
-                radiantPulseReceived(CurOverallSimDay, SurfNum) = (adjQL - curQL) * TMULT(radEnclosureNum) * ITABSF(SurfNum) * Surface(SurfNum).Area;
+        for (int zoneNum = 1; zoneNum <= NumOfZones; ++zoneNum) {// Loop through all surfaces...


Alright, I'm glad CI is showing no diffs to help me feel better about this functional change.

Myoldmopar · 2020-09-02T20:42:20Z

src/EnergyPlus/InternalHeatGains.cc

@@ -5515,7 +5513,7 @@ namespace InternalHeatGains {
        }

        for (Loop = 1; Loop <= TotCO2Gen; ++Loop) {
-            NZ = ZoneCO2Gen(Loop).ZonePtr;
+            int NZ = ZoneCO2Gen(Loop).ZonePtr;


Localizing NZ is great.

Myoldmopar · 2020-09-02T20:45:22Z

src/EnergyPlus/DataSurfaces.cc


    std::vector<int> AllHTSurfaceList;          // List of all heat transfer surfaces
    std::vector<int> AllIZSurfaceList;          // List of all interzone heat transfer surfaces
    std::vector<int> AllHTNonWindowSurfaceList; // List of all non-window heat transfer surfaces
    std::vector<int> AllHTWindowSurfaceList;    // List of all window surfaces
    std::vector<int> AllSurfaceListReportOrder; // List of all surfaces - output reporting order

+    // Surface Window Heat Balance
+    Array1D<Real64> SurfWinTransSolar;                    // Exterior beam plus diffuse solar transmitted through window, or window plus shade/blind, into zone (W)


This is a lot. I know this is one of the parts of EnergyPlus where things are looped over quickly, so I concede that this provides a performance benefit, but just no one get any ideas about doing this in lots of places in the code where we don't do tight loops over array items.

I didn't quite get the sentence after "but". By "doing this in lots of places in the code where we don't do tight loops over array items " do you mean using scalar arrays instead of struct arrays in places with loops that only call a few fields in the original SurfaceWindow struct?

Yes, what is "this" in "this is a lot" referring to?

If what you are referring to is the change from arrays of structs to arrays of scalars, then I don't think it's only a matter of tight loops. Tight loops (i.e., loops with no function calls or conditionals) are what you want for vectorization. But even without vectorization (which I don't think we are getting now) switching to arrays of scalars will improve your cache locality and utilization. If you have structs with a lot of fields that are only used in some calculations, in a given calculation you are probably bringing in a lot of that data unnecessarily. I think this is primarily what we are seeing here. I don't know that this type of transformation (which is really only partial for now) would be effective elsewhere in the code, but I would not dismiss it out of hand.

Myoldmopar · 2020-09-02T21:42:11Z

This runs fine locally. I'll leave this tab open to respond to comments but I have to run for now.

mjwitte

@xuanluo113 Some initial comments.

@JasonGlazer This is likely to conflict with #8196 but oddly not as much as I thought, because many of the fields related to shading (e.g. WindowShadingControlPtr) live on Surface and SurfaceTemp, not on SurfaceWindow. But ShadedConstruction exists in both places - ugh. I'm not finding where SurfWinShadedConstruction(SurfNum) gets set to Surface(SurfNum).ShadedConstruction.

But this didn't throw diffs and it just merged, so . . .

mjwitte · 2020-09-02T21:15:59Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

+        for (int zoneNum = 1; zoneNum <= NumOfZones; ++zoneNum) {
+            int const firstSurfWin = Zone(zoneNum).WindowSurfaceFirst;
+            int const lastSurfWin = Zone(zoneNum).WindowSurfaceLast;
+            if (firstSurfWin == -1) continue;


Hmm, these checks shouldn't be necessary. WindowSurfaceFirst is initialized to zero, and for zones with no windows, WindowSurfaceLast is set to -1 any loops on windowsurfaces will be skipped over. In fact, looking at this now, WindowSurfaceLast and NonWindowSurfaceLast should simply be initialized to -1.

Relying on windowSurfaceFirst to be initialized to 0 and windowSurfaceLast to be initialized to -1 is juuuuuuuust a little too cute. It's a recipe for bugs.

@mjwitte I somehow wrongly pre-assumed this. And -1 is preferable if we want to convert these to vectors eventually? Can I make a follow-up PR to set WindowSurfaceLast and NonWindowSurfaceLast to -1 when no windows/opaqs and revised the checking?

Whatever you do, I would keep the check. Don't get too cute with loop bounds. It's not worth it.

@amirroth As it stand presently, *First and *Last are currently initialized to zero, and then *Last is set to -1 if there are no surfaces in that group. Is there a better way to skip those loops when they are empty?

Yes, set them both to -1 by default (which works for both 1- and 0-based arrays) and do a separate check. A check before a loop whether to do the loop or not is fine, in fact the compiler often inserts checks like that itself. A check on every element in the loop is a lot less fine.

mjwitte · 2020-09-02T21:17:10Z

src/EnergyPlus/DataSurfaces.cc

-    Array1D<Real64> WinTransSolar; // Exterior beam plus diffuse solar transmitted through window, or
-    // window plus shade/blind, into zone (W)
-    Array1D<Real64> WinBmSolar; // Exterior beam solar transmitted through window, or
-    // window plus blind, into zone (W)


Wondering why you renamed arrays that were not previously part of SurfaceWindow?

For the naming convention/prefix we discussed about. SurfWin, SurfOpaq, Surf, and Zone to help understand their scope?

Like, anything starts with SurfWin is expected to be window indexed only and inside firstWindowSurf to lastWindowSurf loop.

Yes, I remember that discussion. I just didn't understand the scope of that went beyond the original SurfaceWindow struct. Not saying it needs to be undone, just didn't expect that.

This convention should be propagated everywhere. The current naming convention "scheme" is terrible. As someone who doesn't actually know what should be going on, let me tell you, it really makes the code difficult to read.

Yes, I remember that discussion. I just didn't understand the scope of that went beyond the original SurfaceWindow struct. Not saying it needs to be undone, just didn't expect that.

:sweat_smile Yes. That went beyond that. Just after converting 30 of those beyond SurfaceWindow, I realized there are another 300 of them that better get renamed and I decided to get this partial effort merged before getting too far.

xuanluo113 · 2020-09-02T22:04:43Z

@mjwitte It's set here. Perhaps only Surface(SurfNum).ShadedConstruction or SurfShadedConstruction(SurfNum) is needed, but what happened here was an automated conversion.

mjwitte · 2020-09-02T22:22:21Z

I see. @JasonGlazer Not sure if you've allowed for multiple blind numbers in #8196 . See here - this block looks like it may need modification to allow for multiple windowshadingcontrols.

JasonGlazer · 2020-09-03T14:29:46Z

@mjwitte I will take a look at the code you pointed out.

xuanluo113 added 7 commits July 7, 2020 11:23

first commit

a81eb78

Merge branch 'develop' of https://github.com/energy-plus/EnergyPlus i…

da706a0

…nto window-test

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into win…

61542a4

…dow-test

test-pass

f16686f

merge

135799e

fix-merge

08ed638

test CI

d229170

xuanluo113 added Performance Includes code changes that are directed at improving the runtime performance of EnergyPlus InProgress Changes to address this feature request have been started labels Jul 8, 2020

xuanluo113 self-assigned this Jul 8, 2020

xuanluo113 added 20 commits July 9, 2020 17:29

fix warning

6e96362

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into win…

5184c14

…dow-test

test

5be0e37

timer

d5b518c

reorder

a56fd89

test

f09182b

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into win…

e912f63

…dow-test

fix

fa47e23

merge

f0757a5

timer

ee4bb10

reorder

d3d5a39

timer

188b195

clean

8137bab

timer

5e3c73d

merge

9d62978

merge

2c35c17

reorder initsolargain

79dc044

reorder

5e76ccf

merge

69eea00

fix

f6fc132

xuanluo113 added 7 commits August 23, 2020 11:07

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into win…

32871b5

…dow-test

revert sh

a8927d1

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into win…

d7c925e

…dow-test

fix

645470c

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into win…

d703a65

…dow-test

fix vars

bb87b0c

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into win…

9d9d060

…dow-test

mjwitte added this to the EnergyPlus 9.4.0 milestone Aug 28, 2020

xuanluo113 added 4 commits August 28, 2020 15:04

window test

568eaf2

SurfWin

912aa93

clean up

4aeb935

clean up

27183f7

xuanluo113 changed the title ~~Scalar SurfaceWindow struct~~ SurfaceWindow struct refactoring Sep 1, 2020

xuanluo113 marked this pull request as ready for review September 1, 2020 16:50

xuanluo113 requested review from Myoldmopar, amirroth and mjwitte September 1, 2020 17:11

xuanluo113 removed the request for review from amirroth September 1, 2020 17:39

Myoldmopar approved these changes Sep 2, 2020

View reviewed changes

Myoldmopar merged commit c3fbac0 into NREL:develop Sep 2, 2020

Myoldmopar deleted the window-test branch September 2, 2020 21:42

mjwitte reviewed Sep 2, 2020

View reviewed changes

This was referenced Sep 8, 2020

Allow Multiple Scheduled Shades to Reference a Single Window #8196

Closed

Allow Multiple Scheduled Shades to Reference a Single Window #8269

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SurfaceWindow struct refactoring #8141

SurfaceWindow struct refactoring #8141

xuanluo113 commented Jul 8, 2020 •

edited

Loading

xuanluo113 commented Sep 1, 2020

amirroth commented Sep 1, 2020 via email

Myoldmopar left a comment

Myoldmopar Sep 2, 2020

Myoldmopar Sep 2, 2020

Myoldmopar Sep 2, 2020

Myoldmopar Sep 2, 2020

Myoldmopar Sep 2, 2020

xuanluo113 Sep 2, 2020

amirroth Sep 2, 2020

amirroth Sep 2, 2020

Myoldmopar commented Sep 2, 2020

mjwitte left a comment

mjwitte Sep 2, 2020

amirroth Sep 2, 2020

xuanluo113 Sep 2, 2020

amirroth Sep 2, 2020

mjwitte Sep 2, 2020

amirroth Sep 2, 2020

mjwitte Sep 2, 2020

xuanluo113 Sep 2, 2020

xuanluo113 Sep 2, 2020

mjwitte Sep 2, 2020

amirroth Sep 2, 2020

xuanluo113 Sep 2, 2020

xuanluo113 commented Sep 2, 2020

mjwitte commented Sep 2, 2020

JasonGlazer commented Sep 3, 2020

SurfaceWindow struct refactoring #8141

SurfaceWindow struct refactoring #8141

Conversation

xuanluo113 commented Jul 8, 2020 • edited Loading

Pull request overview

Pull Request Author

Reviewer

xuanluo113 commented Sep 1, 2020

amirroth commented Sep 1, 2020 via email

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

Myoldmopar commented Sep 2, 2020

mjwitte 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

xuanluo113 commented Sep 2, 2020

mjwitte commented Sep 2, 2020

JasonGlazer commented Sep 3, 2020

xuanluo113 commented Jul 8, 2020 •

edited

Loading