Avoid unnecessary allocations in GetZoneAirLoopEquipment routine every iteration

[nealkruis]

Pull request overview

• Fixes ManageZoneAirLoopEquipment unnecessarily calls GetZoneAirLoopEquipment on every iteration #8962

I expect:

• 6-9% improvement for Mac OS clang builds
• 1-2% improvement for Linux gcc builds
• Unsure about Windows MSVC builds

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

[mitchute]

This looks fine. I'll merge it up with develop one more time then we can merge it.

[mjwitte]

So, this group of seemingly small arrays suck up that much time being needlessly allocated? That's incredible. And these local arrays aren't even necessary. There's state.dataIPShortCut->cAlphaArgs etc. available to use. Not suggesting you need to change that here, but something to watch for elsewhere.

[rraustad]

Be careful! There is a fourPipeBeamFactory call at line 417 that will overwrite the dataIPShortCut variables. So I would think these locals are needed. The only time dataIPShortCut variables can be used is if the getInput in question has no calls to other getInput functions that use the dataIPShortCut variables.

[mitchute]

Sorry @rraustad, I'm confused. Where are you looking?

Here?

EnergyPlus/src/EnergyPlus/ZoneAirLoopEquipmentManager.cc

Lines 414 to 418 in 68b20c8

	state.dataDefineEquipment->AirDistUnit(AirDistUnitNum).airTerminalPtr = FourPipeBeam::HVACFourPipeBeam::fourPipeBeamFactory(
	state, state.dataDefineEquipment->AirDistUnit(AirDistUnitNum).EquipName(1));
	if (state.dataDefineEquipment->AirDistUnit(AirDistUnitNum).UpStreamLeak ||
	state.dataDefineEquipment->AirDistUnit(AirDistUnitNum).DownStreamLeak) {
	ShowSevereError(

[amirroth]

So, this group of seemingly small arrays suck up that much time being needlessly allocated? That's incredible.

They do! I think I mentioned in the most recent 10X "lesson" that std::vector and ArrayXD should not be used as local variables unless absolutely necessary because of the overhead of calling constructors (malloc) and destructors (free).

[rraustad]

I was looking at develop. Yes, that call will getInput the 4pipebeam and overwrite those dataIPShortCut variables mentioned. Low level models that do not do any mining of data can use dataIPShortCut variables (e.g., state.dataIPShortCut->cAlphaArgs). Higher level models that get data from other models usually need to use locals for the getObjectItem call (e.g., Array1D_string AlphArray(5)). This was over 10 years ago so it's not on anyone's radar. Once we switch over to json all these arrays go away.

[rraustad]

As @mjwitte said, there are no changes needed regarding this discussion.

[amirroth]

Be careful! There is a fourPipeBeamFactory call at line 417 that will overwrite the dataIPShortCut variables. So I would think these locals are needed. The only time dataIPShortCut variables can be used is if the getInput in question has no calls to other getInput functions that use the dataIPShortCut variables.

Put a pin in this. Re-entrant code is not a good design pattern. Re-entrant code with variables that are effectively global is really not a good design pattern. This should be "flattened out" somehow.

[mitchute]

I'm not sure what to make of this.

The Windows CI badge is showing a warning, but this isn't a warning over on the CI page. I'm not going to worry about it right now. If there's an actual warning we can clean that up in a later PR. Merging.