Pow, min, and max improved function approximations

[mbadams5]

Pull request overview

This pull request changes pow_* and min/max functions to use new, faster approximations.
For power function, x^n == exp(n*ln(x))
For min/max function, it is using the bitwise operator: min(a,b) == b ^ ((a ^ b) & -(a < b))

Case	Baseline Time/S	Optimized Time/S	Improvement%	Note
5ZoneAirCooled	11.57	10.16	12.187
ASHRAE9012016_HealthCare	250.2	246.6	1.439
ASHRAE9012016_Hospital	448.8	414.6	7.620
ASHRAE9012016_OfficeLarge	159.6	158.4	0.752
ASHRAE9012016_OfficeMedium	31.3	30.77	1.693
ASHRAE9012016_OfficeSmall	30.97	29.49	4.779
ASHRAE9012016_SchoolPrimary	141	140.4	0.426
ASHRAE9012016_SchoolSecondary	261	260.4	0.230
ASHRAE9012016_ApartmentHighRise	460.8	435	5.599
ASHRAE9012016_ApartmentMidRise	71.4	69	3.361
HotelLarge	262.2	262.8	-0.229
HotelSmall	259.8	249.6	3.926
RestaurantFastFood	33.2	31.89	3.946
RestaurantSitDown	1.97	1.84	6.599
RetailStandalone	30.59	30.59	0.000
RetailStipMall	55.03	54.4	1.145
Warehouse	32.27	31.97	0.930
Average			3.200
Weather	USA_NJ_Newark.Intl.AP.725020_TMY3.epw
Version	9.5.0-ea7967b30d
Hardware	MacBook Pro 2017, Catalina 10.15.7, 3.3 GHz Dual-Core Intel Core i5

Overall on average there was a 3.2% performance increase, while some files had up to 12% improvement.

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]

Sweet! I did not know the min(a,b) trick. Does that work on float/double also if you cast them to int?

[nrel-bot-3]

@mbadams5 @lgentile it has been 28 days since this pull request was last updated.

[Myoldmopar]

@amirroth I'm not sure regarding your question above, but it looks like the float functions are still using the more traditional approach. I could look closer, but maybe @mbadams5 could confirm.

I don't have a problem with these changes, but this is definitely needing some love before it's ready. Lots of failing tests and diffs that will need to be analyzed. Do you have a specific plan for getting this on track to merge @mbadams5 ?

[Myoldmopar]

Also interesting that an article was just posted this week on some of these approximations: https://www.geeksforgeeks.org/compute-the-minimum-or-maximum-max-of-two-integers-without-branching/

[DeadParrot]

Fun stuff! I think it would be wise to put this in an E+ math header (in a namespace) rather than altering the ObjexxFCL code so "New & Improved" ObjexxFCL releases can be integrated. (Versions 4.3 and 5.0 with modernizations and performance gains are available.) If you really feel you need to make changes inside the ObjexxFCL code it would be best to flag such changes with a comment to identify the authorship as non-Objexx.

I am skeptical of these "improvements" and there should be demo code proving the performance impact of each function category for each type. Specifically:

• The bit-twiddling min/max may not be faster on all hardware: the ?: ops don't require branching with most optimizing compilers/platforms. The only references I find to this say it is faster "on some rare machines where branching is very expensive and no condition move instructions exist".
  • Edit: Tried this out on Windows: the traditional (?:) method is 15-20% faster with Intel C++ 2021.2 and about the same speed with GCC 11 and VC++2019
• I would be very interested to see small demo code that shows that calling log and exp is faster than a few multiplies for integers(!) and floats -- that seems highly counterintuitive. The point of the pow_* functions is to avoid std::pow for small integer powers. For large enough integer powers it won't be faster but then you are doing a log/exp operation that the compiler writers didn't feel was fast or standard compliant enough to use in their std::pow implementation so it should really have a special name to make it clear that it gives different results and document when it is faster and what the accuracy cost is. This sort of thing should not be slipstreamed into mainstream library calls.
  • Edit: Tried this out also: For pow_3 the exp/log method is massively slower both for double and int arguments. Whatever speedup you are seeing does not appear to be due to these "faster" functions.
• Changing max/min functions to use nested max/min calls instead of the "unrolled" versions has no value in release builds with inlining and just makes debug builds slower.

[nrel-bot-2]

@mbadams5 @lgentile it has been 28 days since this pull request was last updated.

[Myoldmopar]

@mbadams5 do you have anything else on this for now? I haven't tested it myself but it seems like you are seeing improvement and @DeadParrot has done some demo work and is not seeing it. Perhaps some compiler explorer toy tests could help build some confidence either way?

[nrel-bot-3]

@mbadams5 @lgentile it has been 28 days since this pull request was last updated.

[nrel-bot-3]

@mbadams5 @lgentile it has been 28 days since this pull request was last updated.

[Myoldmopar]

@mbadams5 thoughts on this being wrapped up for this release? If not I'll change the milestone to get it out of the review queue.

[nrel-bot-3]

@mbadams5 @lgentile it has been 28 days since this pull request was last updated.

[nrel-bot-3]

@mbadams5 @lgentile it has been 28 days since this pull request was last updated.

[nrel-bot-3]

@mbadams5 @lgentile it has been 28 days since this pull request was last updated.

[nrel-bot-2c]

@mbadams5 it has been 7 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 7 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 8 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 7 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 15 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 9 days since this pull request was last updated.

[nrel-bot-2c]

@mbadams5 it has been 9 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 8 days since this pull request was last updated.

[nrel-bot-2c]

@mbadams5 it has been 14 days since this pull request was last updated.

[nrel-bot-2c]

@mbadams5 it has been 12 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 16 days since this pull request was last updated.

[nrel-bot-2c]

@mbadams5 it has been 10 days since this pull request was last updated.

[nrel-bot-2c]

@mbadams5 it has been 16 days since this pull request was last updated.

[nrel-bot-2c]

@mbadams5 it has been 56 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 9 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 7 days since this pull request was last updated.

[nrel-bot-2c]

@mbadams5 it has been 7 days since this pull request was last updated.

[nrel-bot-2c]

@mbadams5 it has been 7 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 7 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 7 days since this pull request was last updated.

[nrel-bot-2c]

@mbadams5 it has been 7 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 7 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 7 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 7 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 7 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 7 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 7 days since this pull request was last updated.

[nrel-bot-2]

@mbadams5 it has been 7 days since this pull request was last updated.

[nrel-bot-2c]

@mbadams5 it has been 8 days since this pull request was last updated.