Skip to content
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.

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

Performance tweaks and a few bug fixes contributed by Autodesk and an ObjexxFCL update #5151

Merged
merged 8 commits into from Sep 2, 2015

Conversation

DeadParrot
Copy link
Contributor

This is a small set of contributions from Autodesk:

  • Performance refactoring in a few hot spots
  • Bug fixes for undefined variables detected by Clang undefined santizer runs
  • ObjexxFCL updates including an Array1D 2-iterator constructor
  • GCC std lib debug mode define added for debug GCC builds (for detection of things like std::vector bounds violations)

@DeadParrot DeadParrot added Defect Includes code to repair a defect in EnergyPlus Performance Includes code changes that are directed at improving the runtime performance of EnergyPlus labels Aug 27, 2015
@Myoldmopar Myoldmopar removed the Defect Includes code to repair a defect in EnergyPlus label Aug 27, 2015
@Myoldmopar
Copy link
Member

@DeadParrot These changes look really good and clean. CI is so happy... 😉 I'm OK with this merging at will once it comes back all clean unless you have additional work.

@DeadParrot
Copy link
Contributor Author

@Myoldmopar Thanks. I think given the 8.4 release window this is a good place to stop and merge so no additional commits expected.

  • The GCC debug flag probably uncovered a bug in that one Ubuntu case failure: Let me know if you'd like me to debug it as part of this PR
  • I had added the Defect tag because there are some bug fixes in here but it is fine with me to drop it since they are ~minor

@Myoldmopar
Copy link
Member

@DeadParrot Thanks for the response. That failure almost flew past me hidden up there. Interesting failure though:

Error copying file "/home/nrelci/ci/builds/EP-29ddf1242f-x86_64-G4.8ICD/build/Energy+.idd" to "/home/nrelci/ci/builds/EP-29ddf1242f-x86_64-G4.8ICD/build/tst/api_callback/Energy+.idd".

Then the vector subscript error after that. I'm building locally to test it out.

As for the defect tag, the way our changelog is created, the script doesn't want to decide which box to put a merged pull request into, so right now it requires exactly one of five tags on every merged pull request. I'll probably improve that one way, but for now it is working since usually new features are defects are separate (but not always like this one).

@Myoldmopar
Copy link
Member

Looks like this may have had an issue for a while and this just revealed it. I'm testing some solutions locally. @DeadParrot If I figure it out, can I push directly to this branch or do you want me to do it separately then you pull those changes into this branch?

@DeadParrot
Copy link
Contributor Author

@Myoldmopar OK, I'll try to remember to pick just the best tag for each PR. Normally I'd try to avoid mixing performance and bug fixes -- the fixes just fell out of testing for this branch so I bundled them in since it became a kind of grab-bag branch anyway.

Yes, I think the GCC debug flag I added uncovered an existing vector bounds violation. I'm fine with you pushing to this branch if you have a fix. Thx.

@Myoldmopar
Copy link
Member

Problem Found

@DeadParrot OK, I found the problem. It's because when I call E+ as a DLL, I created a null set of command line arguments to pass to the command line argument processor. (The processor is still called to ensure all the right flags are set up inside E+.) However, the command line processor doesn't actually understand this zero-argument condition, and attempts to access the first entry that would normally always be there.

Problem Solved

I made a change in a commit I am pushing shortly that initializes the command line arguments with a single argument as would be expected when calling the program with only the program name and no subsequent arguments. I also had to update the path to the IDD because it couldn't find the idd to copy into the directory.

Solution Review?

  • @nealkruis Could you verify the changes in 66d52ae make sense to you for the CLI option parser you added? I ran it locally and it now works, where I had confirmed it was broken before the fix.
  • CI will also verify this...

Wrap up

If @nealkruis likes the changes, and CI comes back clean, this should be about ready. @DeadParrot it would seem a merge conflict has arisen, so that will ensure the branch is up-to-date anyway, and we can merge.

@DeadParrot
Copy link
Contributor Author

@Myoldmopar Great. Do you want me to fix the merge conflict?

@Myoldmopar
Copy link
Member

Yes, please. I'm off fixing other merge conflicts as we speak. It may be super minor on this branch, I didn't do an attempt.

@DeadParrot
Copy link
Contributor Author

@Myoldmopar Merge conflict cleaned up.

@Myoldmopar
Copy link
Member

Thanks @DeadParrot, this should be good to go once CI is happy.

@nealkruis
Copy link
Member

@Myoldmopar looks good to me.

@Myoldmopar
Copy link
Member

Thanks @nealkruis. OK, @DeadParrot, if I see CI coming back happy after your merge commit, I'll merge this tonight.

@Myoldmopar
Copy link
Member

And there's the result I was looking for the most. IntegrationCoverage now runs all green. Should be mergeable this evening.

@DeadParrot
Copy link
Contributor Author

@Myoldmopar Great. I assume you understand the HeatPumpProportionalControl_DCV diff (extra alpha field?).

@Myoldmopar
Copy link
Member

I assume that will go away now that the branch is up to date with develop.

@Myoldmopar
Copy link
Member

Yep, all green there :)

Myoldmopar added a commit that referenced this pull request Sep 2, 2015
@Myoldmopar Myoldmopar merged commit a59844c into develop Sep 2, 2015
@Myoldmopar Myoldmopar deleted the Autodesk_Performance branch September 2, 2015 12:25
@Myoldmopar
Copy link
Member

@DeadParrot could you rename the pull request so that users understand what came in with the changes? It's merged, thanks!

@DeadParrot DeadParrot changed the title Autodesk performance Performance tweaks and a few bug fixes contributed by Autodesk and an ObjexxFCL update Sep 2, 2015
@DeadParrot
Copy link
Contributor Author

@Myoldmopar Thanks! Renamed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance Includes code changes that are directed at improving the runtime performance of EnergyPlus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants