Fix #9204 - Overcome StdOutputRecordCount's limit of about 2.1 billion #10536

jmarrec · 2024-05-30T15:56:42Z

Pull request overview

Fixes Overcome StdOutputRecordCount's limit of about 2.1 billion #9204

It's pretty much all unit testing... the fix was trivial, the test was clearly not.

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.

``` [ RUN ] EnergyPlusFixture.OutputProcessor_StdoutRecordCount 2005-12-22 02:45 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/OutputProcessor.unit.cc:5483: Failure Expected: (state->dataGlobal->StdOutputRecordCount) > (0), actual: -2147482095 vs 0 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/OutputProcessor.unit.cc:5484: Failure Expected: (state->dataGlobal->StdMeterRecordCount) > (0), actual: -2147483648 vs 0 ```

…4_t) This increases the limit from 2,147,483,648 to 9,223,372,036,854,775,808 records

jmarrec · 2024-05-30T15:57:39Z

src/EnergyPlus/DataGlobals.hh

+    Int64 StdOutputRecordCount = 0;  // Count of Standard output records
+    Int64 StdMeterRecordCount = 0;   // Count of Meter output records


Change StdOutputRecordCount & StdMeterRecordCount to Int64 (std::int64_t)

This increases the limit from 2,147,483,648 to 9,223,372,036,854,775,808 records

Hmmmm. OK, I believe you. I guess that should be big enough 😆

jmarrec · 2024-05-30T15:58:25Z

src/EnergyPlus/ConfiguredFunctions.in.cc

@@ -57,7 +57,7 @@ fs::path configured_source_directory()

 fs::path configured_build_directory()
 {
-    return ("${CMAKE_BUILD_DIR}");
+    return ("${CMAKE_BINARY_DIR}");


Unrelated but was incorrect. I tried to use that to use an actual eso & mtr file instead of the open_as_stringstream to see if the test could work fast enough, but no.

Wow. Good catch.

jmarrec · 2024-05-30T15:58:49Z

tst/EnergyPlus/unit/OutputProcessor.unit.cc

+        // Demonstrate that it's not unreasonable to reach the INT_MAX limit at all
+        constexpr int numOfTimeStepInHour = 60;
+        constexpr int hoursInYear = 8760;
+        constexpr int numOutputVariables = 4200;
+        constexpr size_t totalRecords =
+            static_cast<size_t>(numOfTimeStepInHour) * static_cast<size_t>(hoursInYear) * static_cast<size_t>(numOutputVariables);
+
+        constexpr auto INT_MAX_AS_SIZE_T = static_cast<size_t>(INT_MAX);
+        EXPECT_GT(totalRecords, INT_MAX_AS_SIZE_T);


It's not unreasonable to hit the INT_MAX limit

OK OK I said I believed you.

jmarrec · 2024-05-30T15:59:25Z

tst/EnergyPlus/unit/OutputProcessor.unit.cc

+        // NOTE: the test takes way to long to run if do call UpdateMeterReporting and UpdateDataandReport
+        // So I'm going to just scan for a timestamp were we're about to overflow, and fake it
+        size_t records_written = 0;
+        bool found_about_to_overflow = false;
+        for (state->dataEnvrn->Month = 1; state->dataEnvrn->Month <= 12; ++state->dataEnvrn->Month) {
+            state->dataEnvrn->EndMonthFlag = false;
+            for (state->dataEnvrn->DayOfMonth = 1; state->dataEnvrn->DayOfMonth <= state->dataWeather->EndDayOfMonth(state->dataEnvrn->Month);
+                 ++state->dataEnvrn->DayOfMonth) {
+
+                ++state->dataGlobal->DayOfSim;
+                state->dataGlobal->DayOfSimChr = fmt::to_string(state->dataGlobal->DayOfSim);
+
+                ++state->dataEnvrn->DayOfWeek;
+                if (state->dataEnvrn->DayOfWeek > 7) {
+                    state->dataEnvrn->DayOfWeek = 1;
+                }
+
+                state->dataGlobal->EndDayFlag = false;
+                for (state->dataGlobal->HourOfDay = 1; state->dataGlobal->HourOfDay <= 24; ++state->dataGlobal->HourOfDay) {
+                    for (state->dataGlobal->TimeStep = 1; state->dataGlobal->TimeStep <= state->dataGlobal->NumOfTimeStepInHour;
+                         ++state->dataGlobal->TimeStep) {
+                        if (state->dataGlobal->TimeStep == state->dataGlobal->NumOfTimeStepInHour) {
+                            state->dataGlobal->EndHourFlag = true;
+                            if (state->dataGlobal->HourOfDay == 24) {
+                                state->dataGlobal->EndDayFlag = true;
+                                if ((!state->dataGlobal->WarmupFlag) && (state->dataGlobal->DayOfSim == state->dataGlobal->NumOfDayInEnvrn)) {
+                                    state->dataGlobal->EndEnvrnFlag = true;
+                                }
+                            }
+                        }
+
+                        if (state->dataEnvrn->DayOfMonth == state->dataWeather->EndDayOfMonth(state->dataEnvrn->Month)) {
+                            state->dataEnvrn->EndMonthFlag = true;
+                        }
+                        records_written += numOutputVariables;
+                        if (records_written > (INT_MAX_AS_SIZE_T - numOutputVariables)) {
+                            EXPECT_EQ("2005-12-22 02:45",
+                                      fmt::format("{:04d}-{:02d}-{:02d} {:02d}:{:02d}",
+                                                  state->dataGlobal->CalendarYear,
+                                                  state->dataEnvrn->Month,
+                                                  state->dataEnvrn->DayOfMonth,
+                                                  state->dataGlobal->HourOfDay,
+                                                  state->dataGlobal->TimeStep));
+                            UpdateMeterReporting(*state);
+                            UpdateDataandReport(*state, TimeStepType::Zone);
+                            found_about_to_overflow = true;
+                            break;
+                        }
+                    }
+                    if (found_about_to_overflow) {
+                        break;
+                    }
+                }
+                if (found_about_to_overflow) {
+                    break;
+                }
+            }
+            if (found_about_to_overflow) {
+                break;
+            }
+        }
+        ASSERT_TRUE(found_about_to_overflow);
+        EXPECT_EQ(numOutputVariables + 1, state->dataGlobal->StdOutputRecordCount);
+        EXPECT_EQ(1, state->dataGlobal->StdMeterRecordCount);
+        EXPECT_TRUE(has_eso_output(true));
+        state->dataGlobal->StdOutputRecordCount = records_written;
+        state->dataGlobal->StdMeterRecordCount = INT_MAX;


Fake having written the record for all timesteps until it overflows. if I don't fake it, it's unbearably slow

jmarrec · 2024-05-30T15:59:40Z

tst/EnergyPlus/unit/OutputProcessor.unit.cc

+        ++state->dataGlobal->TimeStep;
+
+        UpdateMeterReporting(*state);
+        UpdateDataandReport(*state, TimeStepType::Zone);
+        EXPECT_GT(state->dataGlobal->StdOutputRecordCount, 0);
+        EXPECT_GT(state->dataGlobal->StdMeterRecordCount, 0);


Get it to overflow over INT_MAX

Myoldmopar

Looks great. And testing locally for kicks.

Myoldmopar · 2024-06-03T17:54:10Z

src/EnergyPlus/ConfiguredFunctions.in.cc

@@ -57,7 +57,7 @@ fs::path configured_source_directory()

 fs::path configured_build_directory()
 {
-    return ("${CMAKE_BUILD_DIR}");
+    return ("${CMAKE_BINARY_DIR}");


Wow. Good catch.

Myoldmopar · 2024-06-03T17:56:03Z

src/EnergyPlus/DataGlobals.hh

+    Int64 StdOutputRecordCount = 0;  // Count of Standard output records
+    Int64 StdMeterRecordCount = 0;   // Count of Meter output records


Hmmmm. OK, I believe you. I guess that should be big enough 😆

Myoldmopar · 2024-06-03T17:56:45Z

tst/EnergyPlus/unit/OutputProcessor.unit.cc

+        // Demonstrate that it's not unreasonable to reach the INT_MAX limit at all
+        constexpr int numOfTimeStepInHour = 60;
+        constexpr int hoursInYear = 8760;
+        constexpr int numOutputVariables = 4200;
+        constexpr size_t totalRecords =
+            static_cast<size_t>(numOfTimeStepInHour) * static_cast<size_t>(hoursInYear) * static_cast<size_t>(numOutputVariables);
+
+        constexpr auto INT_MAX_AS_SIZE_T = static_cast<size_t>(INT_MAX);
+        EXPECT_GT(totalRecords, INT_MAX_AS_SIZE_T);


OK OK I said I believed you.

Myoldmopar · 2024-06-03T17:57:10Z

tst/EnergyPlus/unit/OutputProcessor.unit.cc

+        ++state->dataGlobal->TimeStep;
+
+        UpdateMeterReporting(*state);
+        UpdateDataandReport(*state, TimeStepType::Zone);
+        EXPECT_GT(state->dataGlobal->StdOutputRecordCount, 0);
+        EXPECT_GT(state->dataGlobal->StdMeterRecordCount, 0);


Myoldmopar · 2024-06-03T19:29:22Z

All happy here. Let's merge. Thanks @jmarrec

jmarrec added 4 commits May 30, 2024 12:18

Brute force test for #9204 - but it takes way too long to run

4e24dec

Unrelated (and unused) but configured_build_directory was wrong

b733fae

Change StdOutputRecordCount & StdMeterRecordCount to Int64 (std::int6…

67cf247

…4_t) This increases the limit from 2,147,483,648 to 9,223,372,036,854,775,808 records

jmarrec added the Defect Includes code to repair a defect in EnergyPlus label May 30, 2024

jmarrec self-assigned this May 30, 2024

jmarrec commented May 30, 2024

View reviewed changes

jmarrec requested a review from Myoldmopar May 30, 2024 16:00

Myoldmopar approved these changes Jun 3, 2024

View reviewed changes

Myoldmopar merged commit f795cf3 into develop Jun 3, 2024
17 checks passed

Myoldmopar deleted the 9204-StdOutputRecordCount branch June 3, 2024 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #9204 - Overcome StdOutputRecordCount's limit of about 2.1 billion #10536

Fix #9204 - Overcome StdOutputRecordCount's limit of about 2.1 billion #10536

jmarrec commented May 30, 2024

jmarrec May 30, 2024

Myoldmopar Jun 3, 2024

jmarrec May 30, 2024

Myoldmopar Jun 3, 2024

jmarrec May 30, 2024

Myoldmopar Jun 3, 2024

jmarrec May 30, 2024

jmarrec May 30, 2024

Myoldmopar Jun 3, 2024

Myoldmopar left a comment

Myoldmopar Jun 3, 2024

Myoldmopar Jun 3, 2024

Myoldmopar Jun 3, 2024

Myoldmopar Jun 3, 2024

Myoldmopar commented Jun 3, 2024

		Int64 StdOutputRecordCount = 0; // Count of Standard output records
		Int64 StdMeterRecordCount = 0; // Count of Meter output records

Fix #9204 - Overcome StdOutputRecordCount's limit of about 2.1 billion #10536

Fix #9204 - Overcome StdOutputRecordCount's limit of about 2.1 billion #10536

Conversation

jmarrec commented May 30, 2024

Pull request overview

Pull Request Author

Reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Jun 3, 2024