Replace readItem in ProcessNumber to speed-up SolarShadingTest_ImportedShading #8819

mitchute · 2021-06-13T01:55:59Z

Pull request overview

Addresses Move from UtilityRoutines::ProcessNumber to std::stod #8816
Replaces readItem in ProcessNumber with std::stod.
On my Mac, this resulted in significant runtime reductions for the SolarShadingTest_ImportedShading example file.

Runtimes for SolarShadingTest_ImportedShading
Before
Run 1: 170.00 s
Run 2: 173.31 s
Run 3: 156.62 s

After
Run 1: 15.77 s
Run 2: 15.25 s
Run 3: 10.68 s
Run 4: 11.55 s
Run 5: 12.32 s

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.

…edShading

jmythms · 2021-06-18T15:22:00Z

The run time differences are amazing. I wish we knew what was causing so much improvement in Mac builds, or, what was causing Mac builds to slow down so much with the original code. The runtimes in Linux builds only had an improvement from 8s to 5s. I wonder if it is something to do with the compiler we use for Mac builds, since the code is the same.

mitchute · 2021-06-18T15:32:41Z

I'm not sure what the issue is. readItem is a templated function, so maybe that has something to do with it. Maybe in the future we can move away from readItem for parsing strings. What did you do in the ScheduleManager refactor?

jmythms · 2021-06-18T16:01:26Z

The ScheduleManager refactor is focused on decreasing unnecessary file open, close operations where they can be avoided. I was using the original ProcessNumber function in it. The improvements from this PR will definitely add to the effects of the ScheduleManager refactor.

jmythms · 2021-06-18T16:05:57Z

Since ProcessNumber is used in three places in ScheduleManager.cc, any case that uses those parts of the code should see improvements from this PR:

Schedule:File:Shading
Schedule:Compact
Schedule:File

mjwitte

@mitchute Similar timing results on Windows (just one run each, so not super accurate):
SolarShadingTest_ExternalFraction went from 214s to 113s
SolarShadingTest_ImportedShading went from 118s to 10s

This looks good so far. It would be good to have a unit test that exercises the new catch blocks. I've tested one path with bad values in Schedule;Compact and in the csv (I think), but a unit test that covers all the flavors of numbers this could see including bad values would be useful.

mitchute · 2021-06-18T20:06:24Z

Thanks, @mjwitte. Yes, I was going to do that but it looks like I didn't get it done. I'll work one up.

rraustad · 2021-06-18T20:41:41Z

src/EnergyPlus/UtilityRoutines.cc

-            ErrorFlag = false;
+            try {
+                rProcessNumber = std::stod(PString, nullptr);
+                ErrorFlag = false;


ErrorFlag = false is already set at line 128. Delete line 138. Also, remember the discussion long ago about setting a function argument bool &ErrorFlag to false within any function. It's OK to set it true, but don't set it to false which would erase any previous error. To that end I would delete line 128 as well. @mjwitte.

I am amazed at the impact this has on execution speed!

It looks like there are several places in code where this function is called that relies on ErrorFlag being true or false for each specific call. So do not delete line 128,

tst/EnergyPlus/unit/UtilityRoutines.unit.cc

mitchute · 2021-06-18T21:00:16Z

OK, the unit test has been added. I marked the std::invalid_argument exception to be skipped for coverage results. I can't make it fall in there with any of the following:

"3.14159+0" - returns 3.14159
"3.14159.0" - returns 3.14159
"3.14159+-0" - returns 3.14159
"3.14159-E-0" - returns 3.14159

Maybe we should also be checking for multiple "+", "-", ".", and "E" (or similar, "d", "D", or "e"). It looks like std::stod just reads up to the first place where it finds a valid number and returns that.

mitchute · 2021-06-18T21:08:33Z

Ah, I figured out the invalid_argument exception. "E3.14159" will fall in there. I'll fix that up.

Anything else?

rraustad

This all looks good with a remarkable improvement in execution speed.

rraustad · 2021-06-18T21:21:16Z

OK, the unit test has been added. I marked the std::invalid_argument exception to be skipped for coverage results. I can't make it fall in there with any of the following:

"3.14159+0" - returns 3.14159
"3.14159.0" - returns 3.14159
"3.14159+-0" - returns 3.14159
"3.14159-E-0" - returns 3.14159

Maybe we should also be checking for multiple "+", "-", ".", and "E" (or similar, "d", "D", or "e"). It looks like std::stod just reads up to the first place where it finds a valid number and returns that.

With this speed improvement, wouldn't it be smarter to catch that in the input processor?

mitchute · 2021-06-18T21:32:57Z

wouldn't it be smarter to catch that in the input processor?

I'm honestly not sure what the right path is currently. I think we'll find that this a part of all input processing, regardless of whether it happens in the InputProcessor. readItem is used explicitly all over the place to read in files, including the weather files. I was just looking at a file where my local profiler was saying that readItem was taking something like 20% of a 15-second simulation. We ought to take a look at seeing where we can eliminate it in other places.

rraustad · 2021-06-18T21:33:08Z

Just a thought, could this be moved to the input processor for numeric inputs? That certainly does not have to happen in this branch since all looks good here.

        std::replace_if(
            std::begin(PString), std::end(PString), [](const char c) { return c == 'D' || c == 'd'; }, 'e');

jmythms · 2021-06-21T15:06:06Z

This looks good to me. Testing done:

Checked with Fortran "numerics" like "2d3", the parsing worked well: Converted 2d3 to 2e3 and then to 2000.
Strings like "asdf" changed the ErrorFlag to true.

jmythms · 2021-06-21T15:10:51Z

src/EnergyPlus/UtilityRoutines.cc

+            try {
+                rProcessNumber = std::stod(PString, nullptr);
+            } catch (std::invalid_argument &e) {
+                rProcessNumber = 0.0;
+                ErrorFlag = true;
+            } catch (std::out_of_range &e) {
+                rProcessNumber = 0.0;
+                ErrorFlag = true;
+            }


Looks great!

jmythms · 2021-06-21T15:14:25Z

tst/EnergyPlus/unit/UtilityRoutines.unit.cc

+    badString = "1E5000";
+    expectedVal = 0.0;
+    EXPECT_NEAR(UtilityRoutines::ProcessNumber(badString, expectedError), expectedVal, 1E-5);
+    EXPECT_TRUE(expectedError);


I think this is comprehensive, and touched the cases I could think of.

Myoldmopar · 2021-06-21T15:36:23Z

This is absolutely awesome! I'll look over the code changes now but this has been reviewed by multiple folks and looks like it has a great unit test in place. CI is really happy too.

Myoldmopar

OK this is ridiculous. What an impactful change!!!

Myoldmopar · 2021-06-21T15:38:06Z

I'm going to build and run tests on this locally since I just merged in a couple PRs this morning. If it passes I'll merge. Thanks @mitchute for the work and @rraustad and @mjwitte for the reviews and testing!

Myoldmopar · 2021-06-21T16:44:39Z

All clean with develop merged up. Thanks all.

replace readItem in ProcessNumber to speed-up SolarShadingTest_Import…

8e20beb

…edShading

mitchute added Performance Includes code changes that are directed at improving the runtime performance of EnergyPlus Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring labels Jun 13, 2021

mitchute added this to the EnergyPlus 9.6 Release milestone Jun 13, 2021

mitchute self-assigned this Jun 13, 2021

mitchute requested review from jmythms and mjwitte June 18, 2021 14:57

mjwitte reviewed Jun 18, 2021

View reviewed changes

rraustad reviewed Jun 18, 2021

View reviewed changes

mitchute added 2 commits June 18, 2021 14:48

add unit test, address reviewer comments

1cce149

remove stray backticks

0fd3bb7

rraustad reviewed Jun 18, 2021

View reviewed changes

tst/EnergyPlus/unit/UtilityRoutines.unit.cc Show resolved Hide resolved

rraustad approved these changes Jun 18, 2021

View reviewed changes

final touch-ups after review

5046072

mitchute mentioned this pull request Jun 21, 2021

Eliminate Other Usages of readItem #8834

Merged

20 tasks

jmythms approved these changes Jun 21, 2021

View reviewed changes

Myoldmopar approved these changes Jun 21, 2021

View reviewed changes

Myoldmopar merged commit 68b3b1c into develop Jun 21, 2021

Myoldmopar deleted the replace-readitem-in-processnumber branch June 21, 2021 16:44

Myoldmopar mentioned this pull request Jun 21, 2021

Move from UtilityRoutines::ProcessNumber to std::stod #8816

Closed

mitchute mentioned this pull request Jun 21, 2021

Remove Unused Var6 from PerformanceCurveObject #8798

Merged

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace readItem in ProcessNumber to speed-up SolarShadingTest_ImportedShading #8819

Replace readItem in ProcessNumber to speed-up SolarShadingTest_ImportedShading #8819

mitchute commented Jun 13, 2021

jmythms commented Jun 18, 2021

mitchute commented Jun 18, 2021

jmythms commented Jun 18, 2021 •

edited

Loading

jmythms commented Jun 18, 2021

mjwitte left a comment •

edited

Loading

mitchute commented Jun 18, 2021

rraustad Jun 18, 2021

rraustad Jun 18, 2021

mitchute commented Jun 18, 2021

mitchute commented Jun 18, 2021

rraustad left a comment

rraustad commented Jun 18, 2021

mitchute commented Jun 18, 2021

rraustad commented Jun 18, 2021 •

edited

Loading

jmythms commented Jun 21, 2021 •

edited

Loading

jmythms Jun 21, 2021

jmythms Jun 21, 2021

Myoldmopar commented Jun 21, 2021

Myoldmopar left a comment

Myoldmopar commented Jun 21, 2021

Myoldmopar commented Jun 21, 2021

Replace readItem in ProcessNumber to speed-up SolarShadingTest_ImportedShading #8819

Replace readItem in ProcessNumber to speed-up SolarShadingTest_ImportedShading #8819

Conversation

mitchute commented Jun 13, 2021

Pull request overview

Pull Request Author

Reviewer

jmythms commented Jun 18, 2021

mitchute commented Jun 18, 2021

jmythms commented Jun 18, 2021 • edited Loading

jmythms commented Jun 18, 2021

mjwitte left a comment • edited Loading

Choose a reason for hiding this comment

mitchute commented Jun 18, 2021

rraustad Jun 18, 2021

Choose a reason for hiding this comment

rraustad Jun 18, 2021

Choose a reason for hiding this comment

mitchute commented Jun 18, 2021

mitchute commented Jun 18, 2021

rraustad left a comment

Choose a reason for hiding this comment

rraustad commented Jun 18, 2021

mitchute commented Jun 18, 2021

rraustad commented Jun 18, 2021 • edited Loading

jmythms commented Jun 21, 2021 • edited Loading

jmythms Jun 21, 2021

Choose a reason for hiding this comment

jmythms Jun 21, 2021

Choose a reason for hiding this comment

Myoldmopar commented Jun 21, 2021

Myoldmopar left a comment

Choose a reason for hiding this comment

Myoldmopar commented Jun 21, 2021

Myoldmopar commented Jun 21, 2021

jmythms commented Jun 18, 2021 •

edited

Loading

mjwitte left a comment •

edited

Loading

rraustad commented Jun 18, 2021 •

edited

Loading

jmythms commented Jun 21, 2021 •

edited

Loading