PVWatts Speed Up

[nmerket]

Pull request overview

Fixes #8940

We noticed a performance hit when using PVWatts between v9.2 and v9.5. This was due to the way we're calling out to the SSC library now instead of embedding it in EnergyPlus directly. Apparently there's a noticeable amount of overhead in calling the SSC "compute modules". I've trimmed out what I could. Here are some timings for the residential file I'm working with:

Branch/Tag	Simulation Time (s)
v9.2.0	11
v9.5.0	36
develop	35
pvwatts_speedup	19 14

It's not quite back to 9.2 level speed, but much improved anyway. I called it where I did because I was running out of big time sinks in the profiling. This should have zero diffs, but will run faster for buildings that use the PVWatts model.

cc @shorowit

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 PVWatts unit tests already exist that test the code in question.
• 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

[nmerket]

This lookup of the PVWatts object in the list was adding some time mostly because we called the input processor every time around to get the ObjNum from the object name. Changed the std::map that holds those to have the names as keys instead of ObjNum and it shaved some time off.

[nmerket]

This is probably the biggest win. Using the 1-timestep model from SSC caused the panels to be initialized every timestep. This isn't a design flaw on their end because they take the unchanging properties of the PV system as inputs each timestep, so it could need to be reinitialized, but we know we're not changing those, so I hacked it to only initialize the first time.

[nmerket]

The "compute module" (cmod) in SSC verifies all inputs at each timestep are within valid ranges or enums. We're doing that validation with the idd, so no need to redo that here.

[nmerket]

@amirroth There are typically few PVWattsGenerator objects, but why the aversion to std::map? I mean I could put the PVWattsGenerators in a vector and then loop over them to do the lookup, but why not use a container specifically designed for lookups? When I profile the code the std::map lookup doesn't even register compared to other things.

[amirroth]

@nmerket The real question is not why should you use a std::vector rather than a std::map to do name-based lookups (although for short lists, I am willing to bet that std::vector is still faster than std::map). The real question is why are you doing name-based lookups at all? Why are generators are referenced by name rather than by their position in the generator array? Almost everything else is referenced by index (Surfaces, Zones, Schedules, Components), and of the few remaining things that are referenced by name, most of those should be referenced by index.

[nmerket]

@amirroth If you look at the GeneratorController::simGeneratorGetPowerOutput method, every generator type is doing a name-based lookup. I was taking a "when in Rome" approach. I could see an argument for making a factory function that returns a pointer to the object like the other generator types for consistency.

[amirroth]

@nmerket I was not accusing you of creating this pattern, only of perpetuating it. (Unless of course you also wrote PVWatts itself, in which case I guess I am accusing you of creating it). Either way, please don't double down by perpetuating the class factory pattern also. The factory pattern was created for "impure" object-oriented languages like C++ that don't have first-class types aka virtual constructors. But PVWatts is not even using factory to mimic virtual constructors, so it is not clear what role the factory pattern is playing here other than maybe showing other developers that the developer is cool because he/she has heard of this pattern and also knows how to use dynamic_cast.

Here's the meta-point about map, the factory pattern, and many other things too. Note, this is not specific to you or PVWatts nor is this thread really specific to you or PVWatts, I have made similar comments elsewhere to other developers working on other modules. There is a lot of seemingly cool stuff in C++, STL and other template libraries, and certainly in various OOP and design pattern books. 90% of this stuff has fairly niche uses. I am not saying std::map is never useful, I am saying that it is useful in (many) fewer places than people actually use it (again, I'm not saying usable, I'm saying useful). I am not saying the factory pattern is never useful, I'm saying PVWatts is not using it in the way in which it is itself useful and therefore has no reason to actually use it at all. I understand that there is the temptation to use something because it's new and exciting and you are bored with std::array and std::vector and normal constructors. But for the sake of performance, compile-time, maintainability, readability, etc. it is important to resist this temptation and stick to simple data structures and simple idioms where possible. There are legitimate opportunities to use fancier data structures and patterns, there's no need to manufacture those opportunities and use them where simple data structures and patterns suffice.

[nmerket]

These diffs are just the output variables showing up in a different order. They're all still there.

pvwatts_speedup (nmerket) - x86_64-Linux-Ubuntu-18.04-gcc-7.5: OK (3160 of 3160 tests passed, 1 test warnings)

Messages:\n

* 1 test had: MDD diffs.

* 1 test had: MTD diffs.

* 1 test had: RDD diffs.

[shorowit]

@nmerket This is a big speed improvement relative to v9.5, but I'm surprised that the simulation is still roughly twice as slow as v9.2 Any idea what the remaining slowdown is due to?

[nmerket]

@shorowit Digging a bit deeper into the profiling between versions, we're getting deep into changes in SAM at this point. In the version of the SSC code that ran in v9.2, the function irrad::calc takes up 114 ms of runtime, but in the newer version it takes 6.15s. That's almost the entire difference in runtime. That could be some combination of it being called more times and/or each call takes longer. (It's hard to tell the difference with a sampling profiler, which is all I have access to.) I can't see why it would be calling it more times, though, so I suspect that it's actually taking much longer to run. There are clear differences in the solarpos calculations. This really seems like a speed improvement that will need to be made in SSC upstream.

[Myoldmopar]

Pulled develop in, and fixed the Clang format issue. Built and ran just fine. All tests pass and no unexpected regressions. Only diff is the PVWatts file, which shaved ~60% off on my design day run, even with the overhead of 20 threads running simulations simultaneously. This is a great, high-impact change. If further improvement can be made, cool, but it can happen later. I'm pushing my Clang format fix now and will merge. Thanks @nmerket