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

Add generated types #31

Merged
merged 8 commits into from Aug 27, 2018

Conversation

Projects
None yet
3 participants
@TheDash
Collaborator

TheDash commented Aug 24, 2018

Similar as prior PR but also

  • fixes compiler warnings in test_types.cpp
  • build is now parallelized
  • now builds a libTYPES.so
  • Added some .gitignore items relevant to generated files inside of src/gams/types, as well as other repos/modules that were being pulled in

TheDash added some commits Aug 24, 2018

@TheDash TheDash requested review from dskyle and jredmondson as code owners Aug 24, 2018

@jredmondson

This comment has been minimized.

Owner

jredmondson commented Aug 25, 2018

Time is coming in just under the timeout. Does this look ok @dskyle ?

@dskyle

This comment has been minimized.

Collaborator

dskyle commented Aug 27, 2018

@TheDash I pushed a small change to https://github.com/jredmondson/gams/tree/TheDash-master. Hopefully this will get our build times down a bit. Please merge into your fork, so it's part of this pull request.

@TheDash

This comment has been minimized.

Collaborator

TheDash commented Aug 27, 2018

Build time on my end is about 58~ seconds with 12 cores prior to small fix @dskyle
After small fix im getting 58~.

With 2 cores im getting 2m55s pre-fix. I ran this again and got 2m57s.
With 2 cores im getting 2m58 post-fix. I ran it again (2m59s) because I didn't believe it would be slower

I noticed the .NOTPARALLEL is only for the generation - which is a simple parse and generate read/write and not actual compilation that is not being parallelized. My guess is that it happens so fast that the generation doesn't have an effect on the build time in the scheme (no pun intended) of things.

Thoughts on this? I don't really think its reducing the times for me at all, correct me if I'm wrong/missing something. For others: this could be due to my processor being top notch so serial processing of such a small read/write phase has negligible effects. Compilation on other systems the differences may be noticeable. (see, Travis). I'll commit these changes into my Fork and see what the time diff is on Travis, which seconds matter there (as it inflates to minutes or so, sometimes)

Merge pull request #6 from jredmondson/TheDash-master
Merge in speedup fix to test Travis
@dskyle

This comment has been minimized.

Collaborator

dskyle commented Aug 27, 2018

Thanks, I could be mistaken. Seemed faster to me, subjectively. Also, BTW, I figured out the "always regenerates" issue with capnproto: MPC handles the .c++ extension poorly, escaping in places it shouldn't. I'll have a fix pushed to Madara once CI is done, and then it would be useful to have it applied to GAMS as well. Fix is basically just to include a rename from .c++ to .cpp in the generation command.

@jredmondson

This comment has been minimized.

Owner

jredmondson commented Aug 27, 2018

Are we ready to go with a merge here? If not, what's left? People are waiting on this, e.g. Pawel

@dskyle

dskyle approved these changes Aug 27, 2018

@dskyle dskyle merged commit 0f8d84b into jredmondson:master Aug 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment