Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Multi-dimensional array reordering and refactoring #8692

Merged
merged 7 commits into from
Apr 10, 2021
Merged

Multi-dimensional array reordering and refactoring #8692

merged 7 commits into from
Apr 10, 2021

Conversation

xuanluo113
Copy link
Contributor

@xuanluo113 xuanluo113 commented Apr 3, 2021 •
edited by mjwitte
Loading

Pull request overview

This branch refactors some inefficient use of multi-dimensional array, including:

  • Reordering some 2D window arrays to improve cache locality
  • Change some 2D construction arrays to array of array to avoid slicing and reconstruction.

Pull Request Author

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

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

@xuanluo113 xuanluo113 added the Performance Includes code changes that are directed at improving the runtime performance of EnergyPlus label Apr 3, 2021
@xuanluo113 xuanluo113 self-assigned this Apr 3, 2021
amirroth
amirroth reviewed Apr 3, 2021
View reviewed changes
src/EnergyPlus/General.cc Outdated
@@ -1255,26 +1255,12 @@ Real64 BlindBeamBeamTrans(Real64 const ProfAng, // Solar profile angle (r
}

Real64 POLYF(Real64 const X, // Cosine of angle of incidence
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that in a number of callsites X is 1, I wonder if it makes sense to make this function constexpr.

amirroth
amirroth reviewed Apr 3, 2021
View reviewed changes
src/EnergyPlus/Construction.cc
this->rbBareVisCoef(Layer).dimension(DataSurfaces::MaxPolyCoeff, 0.0);
this->afBareSolCoef(Layer).dimension(DataSurfaces::MaxPolyCoeff, 0.0);
this->abBareSolCoef(Layer).dimension(DataSurfaces::MaxPolyCoeff, 0.0);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably doesn't make much of a difference given that this is done for constructions and only one time, but in general we should move away from combined allocation and initialization using dimension and towards separate allocation and explicit initialization in which all arrays can be initialized together in a single (vectorized) loop.

@amirroth
Copy link
Collaborator

amirroth commented Apr 3, 2021

This is a nice new code pattern! Thanks @xuanluo113!

@xuanluo113 xuanluo113 added this to the EnergyPlus Future milestone Apr 4, 2021
@xuanluo113 xuanluo113 requested a review from mjwitte April 4, 2021 22:58
@xuanluo113 xuanluo113 changed the title Multi dim array Multi dimensional array reordering and refactoring Apr 4, 2021
@xuanluo113 xuanluo113 changed the title Multi dimensional array reordering and refactoring Multi-dimensional array reordering and refactoring Apr 4, 2021
@xuanluo113 xuanluo113 marked this pull request as ready for review April 4, 2021 22:59
mjwitte
mjwitte approved these changes Apr 8, 2021
View reviewed changes
Copy link
Contributor

@mjwitte mjwitte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xuanluo113 I'm ok with these changes. The swapped indexes in some of the 2d arrays is a bit scary, but I guess if any were missed there would be an array bounds error for sure in the CI tests.

Do you want to change the initialization method (per @amirroth comment before this merges?

src/EnergyPlus/HeatBalanceManager.cc
@@ -7206,6 +7204,13 @@ namespace HeatBalanceManager {
state.dataConstruction->Construct(ConstrNum).TotGlassLayers = NGlass(IGlSys);
state.dataConstruction->Construct(ConstrNum).TotSolidLayers = NGlass(IGlSys);

for (int Layer = 1; Layer <= state.dataHeatBal->MaxSolidWinLayers; ++Layer) {
for (int index = 1; index <= DataSurfaces::MaxPolyCoeff; ++index) {
state.dataConstruction->Construct(ConstrNum).AbsBeamCoef(Layer)(index) = 0.0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This syntax will take a while to sink in (for me anyway). x(i)(j)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x[i][j] would make more sense after we change those arrays to vectors.

src/EnergyPlus/HeatBalanceManager.cc
@@ -6234,8 +6234,8 @@ namespace HeatBalanceManager {
if (thisSurface.Class == DataSurfaces::SurfaceClass::Window) {
auto &thisConstruct(thisSurface.Construction);
if (!state.dataConstruction->Construct(thisConstruct).WindowTypeBSDF) {
state.dataHeatBal->SurfWinFenLaySurfTempFront(1, SurfNum) = state.dataHeatBalSurf->TH(1, 1, SurfNum);
state.dataHeatBal->SurfWinFenLaySurfTempBack(state.dataConstruction->Construct(thisConstruct).TotLayers, SurfNum) =
state.dataHeatBal->SurfWinFenLaySurfTempFront(SurfNum, 1) = state.dataHeatBalSurf->TH(1, 1, SurfNum);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that these arrays (and many others) were originally (pre-c++) x(SurfNum, LayerNum) (or whatever), but back in #4383, they were reversed (find SolarShading.cc and expand it then the lines should show there) to be row-major, and now we're going back to the original order.

src/EnergyPlus/WindowManager.cc
state.dataConstruction->Construct(ConstrNum).tBareSolCoef(_, IGlass));
state.dataConstruction->Construct(ConstrNum).tBareSolCoef(IGlass));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always nice to get rid of _ ,

@xuanluo113
Copy link
Contributor Author

@mjwitte I changed the initialization method in the construction array initialization. More on this topic, I'm switching to a branch to convert surface object fields to surface HB arrays. I found the location of initialization of dataSurface, dataHeatBal, dataHeatBalSurf, dataHeatBalSurfMgr arrays are sometimes random (some in SurfaceGeometry, some in SolarShading, some in HBManger or HBSurfaceManager), and it's a little misleading to justify where to add new surface HB arrays. I'll probably revisit the initialization issue later.

@mjwitte mjwitte merged commit ad259e8 into NREL:develop Apr 10, 2021
@mjwitte mjwitte deleted the multi-dim-array branch April 10, 2021 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amirroth amirroth amirroth left review comments

@mjwitte mjwitte mjwitte approved these changes

Assignees

@xuanluo113 xuanluo113

Labels
Performance Includes code changes that are directed at improving the runtime performance of EnergyPlus
Projects
None yet
Milestone
EnergyPlus 9.6 Release
Development

Successfully merging this pull request may close these issues.
8 participants
@xuanluo113 @amirroth @mjwitte @nrel-bot @nrel-bot-2 @nrel-bot-3 @nrel-bot-2b @nrel-bot-2c