Surf heat balance arrays refactoring

[xuanluo113]

Pull request overview

This branch including the refactoring of moving run time heat balance variables from SurfaceData object to surface arrays. The following refactorings are also made along with the restructuring.

1. Put GetMovableInsulationData and GetShadingSurfReflectanceData after Surface is allocated and sorted. The related property arrays do not need to be assigned to SurfaceTmp array and copied to Surface later.
2. Move Encl arrays to DataHeatBal namespaces.
3. Remove dataHeatBalSurf->ZoneMRT arrays withthe same meaning as dataHeatBal->ZoneMRT arrays to reduce data copies at each time step.
4. Naming convention of surface and zone arrays in DataSurface and DataHeatBal namespaces (e.g. QS -> EnclSolSWRad, QL -> EnclRadQLWRad).
5. Reduce run time allocation checking such as state.dataSolarShading->MustAllocSolarShading or if (!allocated(state.dataHeatBal->EnclSolQSWRad)) and (conditionally) allocate the arrays once during simulation setup. These conditions do not have to be checked at every time step.
6. Convert the third party function call sum_product_sub(state.dataSurface->Surface, &SurfaceData::Area, &SurfaceData::OutDryBulbTemp, SurfPtrARR) to manual calc of sum product (as SurfOutDryBulbTemp field is not in SurfaceData domain).
7. Some function calls to pass Surface Object pointer (&surface) -> pass SurfNum (index only).

The CI performance is not green. Other than items 6 & 7 I don't see what other changes may possibly reduce the performance.

There are two parts of text diffs causing by:

1. Reporting state.dataSurface->SurfExtConvCoeff(SurfLoop) instead of Surface(SurfLoop).ExtConvCoeff changes the eio printout from 0.0 to 0 for zero values.

 print(state.files.eio,
      Format_901,
      Surface(SurfLoop).Name,
-     Surface(SurfLoop).ExtConvCoeff,
+     state.dataSurface->SurfExtConvCoeff(SurfLoop),

2. Setup of output variable "Zone Mean Radiant Temperature" is moved from DataHeatBalSurf to DataHeatBal, causing the reordering of the RDD files.

There are still quite a few related array reordering, renaming and initialization issue to fix including:

1. The todos marked in DataHeatBalance and DataHeatBalanceSurf
2. Avoid using dimension for initialization.
3. Naming of NumOfZones, NumOfSolarEnclosures, etc. (in comparing to TotSurfaces?)
   that I plan to do in some other branch later.

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

[amirroth]

I don't think we should do this type of formatting at this time. IMO, it would be better to leave the lines as one and to shorten some of the variable and field names, .e.g.,

s.surface->OutDryBulbTemp(SurfNum) = s.loopNodes->Node(s.surface->LinkedOutAirNode(SurfNum)).OutAirDryBulb;

[amirroth]

We should avoid using these array initializers and initialize explicitly using a loop. This should also allow us to combine these different initialization loops into a single loop.

[amirroth]

Also, should use a symbolic constexpr rather than a literal 23.0.

[amirroth]

It's shorter and clearer to write int than auto.

[amirroth]

Why is IsShadowing a surface-level array while HeatTransSurf and ExtSolar are fields in the Surface array? It seems like these fields have similar usages.

[xuanluo113]

@amirroth HeatTransSurf and ExtSolar are assigned during input processing and saved in SurfaceTmp (e.g.ExtSolar = UtilityRoutines::SameString(state.dataIPShortCut->cAlphaArgs(ArgPointer), "SunExposed")). With all surface data being read, SurfaceTmp arrays are sorted and copied to a new Surface array. So these fields remain as fields not arrays because otherwise, I'll have to sort the arrays. IsShadowing is evaluated and assigned afterward. I don't know if it's worthwhile or good practice to move it out from a field to an array.

[amirroth]

I am not suggesting removing it from the class, I was suggesting replicating it in its own array if the use cases warrants it. If not (maybe surfaces are sorted so that you don't have to explicitly check for these conditions or maybe we are using explicit surface lists that only include surfaces with these conditions) then it doesn't matter.

[amirroth]

FWIW I would also like us to stop using dimension and replace with allocate and explicit loop initialization. This will allow us to combine the initializations multiple arrays into a single loop, which improves performance.

[xuanluo113]

@mjwitte This branch is ready for review. The PR overview lists a walkthrough of the changes and diffs. The CI performance is not green... but tested locally 10x_incr_100zones100surfaces.idf shows a 8% run time reduction comparing to the current develop head. For other models smaller than this, the run time difference is marginal.
I've addressed some of @amirroth 's comments above. There are some remaining related issues either Amir mentioned or I plotted, and I plan to start another branch to continue working on (as listed in the overview).
Thanks very much!

[amirroth]

Hmmm. Surprising that you are not seeing any improvements on other models, although 8% on a big model is good. Maybe some other things like data-dependent branches are getting in the way of the speedups. Which functions are you expecting to see speedup on?

[mjwitte]

Since a single instance of Surface is a parameter to this lambda function, is it possible that the lookup of state.dataSurface->SurfShadowPossibleObstruction(ObsSurfNum) is more work than fetching surface.ShadowSurfPossibleObstruction?

[amirroth]

It is. However, given that this function was written when all data fields were part of the SurfaceData struct, it is also possible that this lambda needs to be written to take a SurfIdx argument rather than a Surface & argument. Either way, I don't think this is a huge immediate problem.

[mjwitte]

The reason I asked about this is because the performance test for 15zonePSZ shows about 38 million more "cost" on this branch than develop (0.24% more). About 10m is coming from daylighting functions. Small overall, but just curious why these code changes would result in any increase at all.

[xuanluo113]

Yes. I found this piece suspicious when I rewrote this part to fit the new array structure into the lambda function. I actually had to add an Index to Surface to find the SurfNum. (Potentially Index should be useful as there are still places we find Surface Index by Name using FindItemInList)
I didn't realize it would create such performance reduction. I actually thought about rewriting the lambda function, but the syntax at this part looked quite confusing to me. Since it's potentially a performance bottleneck I'll take another look.

[amirroth]

I see. This one change shouldn't be that much, unless daylighting functions are called much more frequently than they need to be. Overall, the increase in instruction count probably has to do with the fact that we have to call many more array constructors & initializers. Some of that will go away if we initialize explicitly using one or several large loops as opposed to using dimension. Definitely things to keep markers on.

[amirroth]

A few things.

1. It's fine to have both a SurfShadowPossibleObstruction array and a SurfShadowPossibleObstruction field. If they don't change over the life of the model, then there is no overhead associated with keeping them in sync.
2. It's also probably fine to leave SurfShadowPossibleObstruction as a field since I don't imagine it is used in the kinds of loops that would be amenable to vectorization anyway.
3. You could change the PierceSurface/SurfaceOctree functions to take SurfIndex instead of Surface & as an argument. If we are worried about the performance of those functions there are possibly other things we can do to make them faster, not sure if these mods belong in this branch/PR.

[xuanluo113]

@mjwitte So I reverted the changes of two shadowing fields to arrays. Testing locally before and after using 15zonePSZ, comparing develop and this branch, I can't see any meaningful and consistent performance difference. I also used local timers to test the daylighting functions, and the run time difference is marginal (<0.5%). I'll wait for CI to tell the updated performance difference then.

[mjwitte]

These should be integers. That should reduce the eio diffs. Without seeing the declarations, I had assumed SurfIntConvCoeff was the actual convection coefficient value. Perhaps these should be renamed, e.g. SurfIntConvCoeffIndex ?

[mjwitte]

@xuanluo113 I pushed up a few minor doc edits to clean up font and equation width issues (that were present before but were more pronounced with the longer variable names). Once CI comes back clean, I will merge this.