New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #9550 - Avoid expensive calls inside BuildKeyVarList #9556

Merged

Myoldmopar merged 15 commits into develop from BuildKeyVar_perf

Aug 2, 2022

Contributor

jmarrec commented Jul 22, 2022 •

edited

Loading

Pull request overview

Fix HospitalBaseline.idf sometimes times out due to expensive calls inside BuildKeyVarList #9550
Avoid expensive calls inside BuildKeyVarList

Please see #9556 (comment) for explanations.

time ./Products/energyplus -w $chig_epw -d out-delete -D -r ../EnergyPlus/testfiles/HospitalBaseline.idf

Build Type	develop	PR	Runtime
Debug	180.644	120.531	-33%
Release	14.6902	11.715	-20%

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.

jmarrec commented

View reviewed changes

src/EnergyPlus/OutputProcessor.hh

Comment on lines +436 to +437

		bool is_simple_string; // Whether the Key potentially includes a Regular Expression pattern
		std::shared_ptr<RE2> case_insensitive_pattern;

Contributor Author

jmarrec Jul 22, 2022 •

edited

Loading

ReqReportVariables: during GetInput, we try to see if the Keyis a regex or not, and we store: a pre-compiled RE2 case insensitive pattern, and a boolean flag to indicate whether it's supposed to be a Regex or a normal match.

Note: the boolean could be omitted by just checking case_insensitive_pattern == nullptr. I'm loosely following what DataOutputs.cc does.

EnergyPlus/src/EnergyPlus/DataOutputs.hh

Lines 74 to 83 in 6e02a12

    
           struct OutputReportingVariables 
        
           { 
        
               OutputReportingVariables(EnergyPlusData &state, std::string const &KeyValue, std::string const &VariableName); 
        
               std::string const key; 
        
               std::string const variableName; 
        
               bool is_simple_string = true; 
        
               std::shared_ptr<RE2> pattern = nullptr; 
        
               std::shared_ptr<RE2> case_insensitive_pattern = nullptr; 
        
           };

src/EnergyPlus/OutputProcessor.cc

Comment on lines +315 to +324

+                      for (int i = 1; i <= op->NumOfReqVariables; ++i) {
+                          auto &reqRepVar = op->ReqRepVars(i);
-                      // SUBROUTINE LOCAL VARIABLE DECLARATIONS:
-                      int Loop;
-                      int Loop1;
-                      bool Dup;
-                      auto &op(state.dataOutputProcessor);
-                      for (Loop = MinIndx; Loop <= MaxIndx; ++Loop) {
-                          if (op->ReqRepVars(Loop).Key.empty()) continue;
-                          if (!UtilityRoutines::SameString(op->ReqRepVars(Loop).VarName, VariableName)) continue;
-                          if (!(UtilityRoutines::SameString(op->ReqRepVars(Loop).Key, KeyedValue) || RE2::FullMatch(KeyedValue, "(?i)" + op->ReqRepVars(Loop).Key)))
+                          if (!UtilityRoutines::SameString(reqRepVar.VarName, VarName)) {
                               continue;
-                          //   A match.  Make sure doesn't duplicate
-                          op->ReqRepVars(Loop).Used = true;
-                          Dup = false;
-                          for (Loop1 = 1; Loop1 <= op->NumExtraVars; ++Loop1) {
-                              if (op->ReqRepVars(op->ReportList(Loop1)).frequency == op->ReqRepVars(Loop).frequency) {
-                                  Dup = true;
-                              } else {
-                                  continue;
-                              }
-                              //  So Same Report Frequency
-                              if (op->ReqRepVars(op->ReportList(Loop1)).SchedPtr != op->ReqRepVars(Loop).SchedPtr) Dup = false;
                           }
-                          if (!Dup) {
-                              ++op->NumExtraVars;
-                              if (op->NumExtraVars == op->NumReportList) {
-                                  op->ReportList.redimension(op->NumReportList += 100, 0);
-                              }
-                              op->ReportList(op->NumExtraVars) = Loop;
+                          if (!reqRepVar.Key.empty() && !(reqRepVar.is_simple_string && UtilityRoutines::SameString(reqRepVar.Key, KeyedValue)) &&
+                              !(!reqRepVar.is_simple_string && RE2::FullMatch(KeyedValue, *(reqRepVar.case_insensitive_pattern)))) {
+                              continue;

Contributor Author

jmarrec Jul 22, 2022

Originally the array was traversed multiple times:

A first time to check the first and last index that match the VarName
A second time partially, between that min and max index in BuildKeyVarList (filtering out blank keys, and indices where VarName didn't match)
A third time partially, between that min and max index in AddBlankKeys (filtering out non blank keys, and indices where VarName didn't match)

The other and main thing was with the RE2 in BuildKeyVarList

Anytime a Key didn't match "normally", it would build a single use RE2 pattern again (which is very expensive) and try to match, regardless of whether the Key contained any special characters making it a potential regex
That meant that a single Output:Variable could lead to the creation of potentially hundreds or thousands of RE2 patterns
- HospitalBasline is a good (albeit extreme) example because it has a lot of variables that are exact-keyed for node variables, eg Output:Variable,Floor 1 Cafe VAV Box Outlet Node Name,System Node Standard Density Volume Flow Rate,hourly;.
- Each Node will end up triggering a call to SetupOutputVariable for System Node Standard Density Volume Flow Rate and every time (except that one time where the Key matches exactly in the first pass) that single Output:Variable will lead to the creation of an expensive RE2 pattern
- There are 130 System Node Standard Density Volume Flow Rate variables requested in that file. 1420 unique node names are in that file. So there are 1420 * (130 - 1) = 183 180 RE2 patterns being created, versus zero with this PR.

time ./Products/energyplus -w $chig_epw -d out-delete -D -r ../EnergyPlus/testfiles/HospitalBaseline.idf

Build Type	develop	PR	Runtime
Debug	180.644	120.531	-33%
Release	14.6902	11.715	-20%

Collaborator

amirroth Aug 2, 2022

🔥 🔥 🔥

src/EnergyPlus/OutputProcessor.cc

                                   Dup = true;
-                              } else {
-                                  continue;
+                                  break;

Contributor Author

jmarrec Jul 22, 2022

no point keeping looking as soon as we've found it's a Dup.

src/EnergyPlus/OutputProcessor.cc Outdated Show resolved Hide resolved

src/EnergyPlus/OutputProcessor.cc

Comment on lines +473 to +471

		// TODO: I think it's supposed to be upper case already, but tests aren't doing that at least...
		const std::string FreqStringUpper = UtilityRoutines::MakeUPPERCase(FreqString);

Contributor Author

jmarrec Jul 22, 2022

I wasn't sure if this was really true (and then I should just modify the tests to be upper case as well) or not.

src/EnergyPlus/OutputProcessor.cc

    
            @@ -558,10 +444,12 @@ namespace OutputProcessor {
          
                      // SUBROUTINE ARGUMENT DEFINITIONS:

                      // SUBROUTINE PARAMETER DEFINITIONS:

                      static std::vector<std::string> const PossibleFreq({"deta", "time", "hour", "dail", "mont", "runp", "envi", "annu"});

                      static std::vector<std::string> const PossibleFreqs({"DETA", "TIME", "HOUR", "DAIL", "MONT", "RUNP", "ENVI", "ANNU"});

Contributor Author

jmarrec Jul 22, 2022

Avoid consistently upper casing stuff.

Collaborator

amirroth Aug 2, 2022

I know this is already merged, but ... these things can/should be static constexpr std::array<std::string_view> which are compiled into the executable rather than constructed (I understand that this is static and so only constructed once, but once is still more than zero times.

src/EnergyPlus/OutputProcessor.cc Outdated Show resolved Hide resolved

src/EnergyPlus/OutputProcessor.cc Outdated

+                          bool is_simple_string = !UtilityRoutines::isKeyRegexLike(reqRepVar.Key);
+                          reqRepVar.is_simple_string = is_simple_string;
+                          if (!is_simple_string) {
+                              reqRepVar.case_insensitive_pattern = std::make_shared<RE2>("(?i)" + reqRepVar.Key);

Contributor Author

jmarrec Jul 22, 2022

Check if this has special regex chars, if so compile the pattern and store.

src/EnergyPlus/UtilityRoutines.cc Outdated Show resolved Hide resolved

jmarrec commented

View reviewed changes

src/EnergyPlus/DataOutputs.cc Outdated Show resolved Hide resolved

src/EnergyPlus/OutputProcessor.cc

Comment on lines +315 to +316

		for (int i = 1; i <= op->NumOfReqVariables; ++i) {
		auto &reqRepVar = op->ReqRepVars(i);

Contributor Author

jmarrec Jul 22, 2022

@amirroth 's comment on a commit (which is much harder to see / find) here: bd6ce05#r78895379

📌 Most EnergyPlus uses of std::map or std::unordered_map are misguided, i.e., a map is overkill because the expected number of elements is small and/or the elements are compile time constants which maps cannot be. Then there are situations like this which look like good fits for a map--potentially large number of dynamic items--and we are rolling with an Array1D.

jmarrec added 14 commits

August 1, 2022 09:20


          Adjust OutputProcessor tests to match

40f1ace


          Initial readability: take a reference

5cba00e


          Use a std::find_if (the previous dup loop was also missing a break)

5a42a30

0m24,836s => 0m23,870s


          Do it in two pass: Key match first, then for remaining try a regex: r…

65d09dc

…egex are EXPENSIVE

0m24,836s => 0m23,870s => 0m19,816s


          Fixup: can't use a range based for loop on ReportList, it's allocated…

047710f

… to a big size


          Avoid more work

9e66e27


          Avoid traversing arrays several times to do similar issues (delete Bu…

e844c0a

…ildKeyVarList/AddBlankKeys), check if a Key could be a Regex and cache an RE2 case insentitive pattern


          Adjust tests with the new is_simple_string attribute

6d0a096


          Unrelated: right trim when a closing bracket is found for Variable Name

0dccd1d


          Remove comment

6ebfe36


          Move the DataOutputs is_simple_string determination into the UtilityR…

ec075e1

…outines so I can test against it


          Add a test for isRegexKeyLike/isRegexKeyLikeOri

498da58


          My version needs to check for "*" (exact match), and also consider th…

f90bced

…e `.` (dot)


          Move the isKeyRegexLike implementation in DataOutputs to avoid pollut…

…ing UtilityRoutines

jmarrec force-pushed the BuildKeyVar_perf branch from 11e0d6f to 2494907 Compare

August 1, 2022 08:39

jmarrec commented

View reviewed changes

tst/EnergyPlus/unit/OutputProcessor.unit.cc Outdated

Comment on lines 6072 to 6090

+                  TEST_F(EnergyPlusFixture, DataOutputs_isKeyRegexLike)
+                  {
+                      std::vector<std::pair<std::string, bool>> test_cases{
+                          {"*", false},
+                          {"This is the first one", false},
+                          {"This is another.*one", true},
+                          {"This is (a|some) ones?", true},
+                          {"Zone 1.1", true},   // The `.` could mean any character (though in this case it's not meant as a regex really)
+                          {"Cafétéria", false}, // !std::isalnum, but not a regex
+                      };
+                      for (auto &[s, expectedIsRegexLike] : test_cases) {
+                          EXPECT_EQ(expectedIsRegexLike, DataOutputs::isKeyRegexLike(s)) << "isKeyRegexLike: Failed for " << s;
+                      }
+                      for (auto &[s, expectedIsRegexLike] : test_cases) {
+                          EXPECT_EQ(expectedIsRegexLike, DataOutputs::isKeyRegexLikeOri(s)) << "isKeyRegexLikeOri: Failed for " << s;
+                      }
+                  }

Contributor Author

jmarrec Aug 1, 2022 •

edited

Loading

The original implementation fails for Cafétéria (assumes it's a regex when there's no chance it is). isKeyRegexLikeOri is extracted from

EnergyPlus/src/EnergyPlus/DataOutputs.cc

Lines 141 to 146 in 2a26c7b

    
           if (KeyValue == "*") return; 
        
           for (auto const &c : KeyValue) { 
        
               if (c == ' ' || c == '_' || std::isalnum(c)) continue; 
        
               is_simple_string = false; 
        
               break; 
        
           }

)

[ RUN      ] EnergyPlusFixture.DataOutputs_isKeyRegexLike
/home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/OutputProcessor.unit.cc:6088: Failure
Expected equality of these values:
  expectedIsRegexLike
    Which is: false
  DataOutputs::isKeyRegexLikeOri(s)
    Which is: true
isKeyRegexLikeOri: Failed for Cafétéria

Contributor Author

jmarrec Aug 1, 2022

Test bed: https://godbolt.org/z/Y4n74orbK

Collaborator

amirroth Aug 2, 2022

Let me ask a high-level question. To what extent do EnergyPlus users actually use general-purpose regex for variable keys? Or do they mostly use just plain *?

Contributor Author

jmarrec Aug 3, 2022

Can you define "general-purpose" please? Not sure I follow.

Collaborator

amirroth Aug 3, 2022

I guess I meant "general" rather than "general-purpose". Basically, anything other than a simple "*" with nothing else around it. If those are the majority of the regex use cases, then you can probably create a special case for it that doesn't use regex at all.

Contributor Author

jmarrec Aug 3, 2022

so you mean .* or .+? I doubt people use that, they just use * then.

I'd bet most use case are prefix/suffix filtering. Eg if you name your zones FLOOR1-Zone1 etc, you can get repporting for FLOOR1-.*. Same if you named the nodes of a loop like LOOPNAME NODENAME

Collaborator

amirroth Aug 3, 2022

A prefix is also a special case that doesn't need the regex library.

Collaborator

amirroth Aug 3, 2022

I don't know if you know what I am getting at. If the cost is in compiling the regex and the common usages are special cases that can be handled using has_prefix, has_suffix, or even no explicit matching at all (*), then we can reduce the frequency of regex compilation even further. I'm going to just leave this here. Maybe what you've already done (:fire:) is the 80/20 sweet spot here and no additional optimization is needed at this time.


          Use my isKeyRegexLike implementation

ee41ce7

jmarrec requested a review from Myoldmopar

August 1, 2022 09:24

jmarrec added the Defect label

jmarrec self-assigned this

jmarrec marked this pull request as ready for review

August 1, 2022 09:24

Member

Myoldmopar commented Aug 1, 2022

@jmarrec this is an incredible improvement, thanks! Looks like there is still one more failing unit test, but otherwise CI is happy. Let me know if you need anything from me to help resolve that, but I'm assuming it's a straightforward tweak to get it working. Thanks!

Myoldmopar added this to the EnergyPlus 22.2 milestone

Contributor Author

jmarrec commented Aug 2, 2022

@Myoldmopar I don't know why decent CI does that, but the build results posted here are just for an older commit 2494907 (that I pushed on purpose with a failing test). Maybe it's because I immediately pushed the fix right after and decent ci has a kind of time delay?

Anyways, if you click on the latest commit you'll see all green results, aside from the SolarShading GPU test usual issue (we must address it or disable that test, or just rerun it if it fails or something.... CI should have a 100% pass rate all the time unless there is a change). see ee41ce7

Member

Myoldmopar commented Aug 2, 2022

You're right, all clean! Not sure why it got confused, but this is green and ready to go in. Thanks!!

Myoldmopar merged commit a962e75 into develop

Myoldmopar deleted the BuildKeyVar_perf branch

August 2, 2022 18:31

jmarrec mentioned this pull request

Allow for regex expressions in output variable names, specifically for construction node values #9632

Open

2 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Defect