Faster compilation #853

pcarruscag · 2020-01-29T10:06:25Z

Proposed Changes

Addresses the compilation time issue (#851) in the way discussed with @talbring in #824.
Plus, I think it is time to bite the bullet and cleanup CConfig, no more .inl, tabs replaced (but the comments still look ok-ish), methods made const when possible.
This will probably cause you issues, but the changes to these files are usually very linear and so, easy to re-apply.
There is no more CCreateMap function, in C++11 braced initialization can be used instead e.g.:
map<string, string> AverageGroupName = {{"BGS_RES", "bgs"},{"RMS_RES","rms"},{"MAX_RES", "max"}};

PR Checklist

I am submitting my contribution to the develop branch.
My contribution generates no new compiler warnings (try with the '-Wall -Wextra -Wno-unused-parameter -Wno-empty-body' compiler flags).
My contribution is commented and consistent with SU2 style.
I have added a test case that demonstrates my contribution, if necessary.
I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp) , if necessary.

restructure and clean config_structure

pcarruscag · 2020-01-29T10:13:07Z

Common/include/option_structure.hpp

-static const map<string, AVERAGE_TYPE> Average_Map = CCreateMap<string, AVERAGE_TYPE>
-("AREA", AVERAGE_AREA)
-("MASSFLUX", AVERAGE_MASSFLUX);
+static const MapType<string, AVERAGE_TYPE> Average_Map = {
+  MakePair("AREA", AVERAGE_AREA)
+  MakePair("MASSFLUX", AVERAGE_MASSFLUX)
+};


This is what the map declaration semantics look like now, so that they can be switched on/off.
When map declarations are needed #define ENABLE_MAPS before including either config/option_structure.hpp, by default this is not default.
MakePair is a "do-nothing" macro when that flag is not defined, MapType is defined at the top as either std::map or a dummy empty class when the flag is not defined.

talbring · 2020-01-29T10:25:27Z

Thanks @pcarruscag ! FYI: that makes compilation like 5 times faster on our system ...

…compilation

TobiKattmann · 2020-01-29T11:31:45Z

Thanks @pcarruscag ! FYI: that makes compilation like 5 times faster on our system ...

Yeah, the speedup is ridiculous. On 28cores on our system an MPI build (no AD-stuff involved) takes ~30 seconds instead of 3min. 🐎 vs 🐌
Great job!

talbring

Maybe its a good thing to clean up config_structure now ... We can also think about renaming it (that should be tracked by git anyway).

SU2_DOT/include/SU2_DOT.hpp

pcarruscag · 2020-01-29T14:06:46Z

I already did the spaces, the tabs, the inlines, what more is there?

talbring · 2020-01-29T14:18:31Z

I already did the spaces, the tabs, the inlines, what more is there?

Just wanted to say that I think its good that you did that 👍

pcarruscag · 2020-01-29T14:59:01Z

We could always look for unused methods amongst the thousand or so we have 🤣

economon · 2020-01-29T15:56:33Z

Nice job on the compile speed up @pcarruscag 👍

economon · 2020-01-29T16:14:22Z

Common/include/option_structure.hpp

@@ -174,34 +147,35 @@ enum AVERAGE_TYPE {
  AVERAGE_AREA = 1,     /*!< \brief Area-weighted average. */


While major changes are happening in this file, it could be a good time to finally remove the integer specifiers for each value in the enums, which aren't needed. Up to you if you have time

Hmmm If we did not cast every enum to unsigned short in CConfig I would agree, and it should be doable with a script. But for debugging I find it useful to know what number to expect without having to count / print extra stuff.

jayantmukho

Changes seem to be pretty straight forward. Got a speedup of ~3.8x for compilation with AD and pywrapper. (meson+ninja)

Since the config has been restructured, you could change the file name to CConfig.

pcarruscag · 2020-01-31T08:31:25Z

Files renamed

talbring

Everything looks fine! Lets get that in asap. Thanks @pcarruscag once again ..

TobiKattmann

I have some very picky comments only related to comments and alignment so you might as well ditch them.
I would have preferred though to separate the CConfig refactor + and cleanup and the compile time stuff in different PR's to improve PR's readability.
But anyway, the compile time improvement is awesome and the cleanup goes beyond just stripping whitespaces but includes alignements, typo-fixes, the const-additions etc ... so thanks a lot!

TobiKattmann · 2020-01-31T07:39:29Z

Common/include/config_structure.hpp

+
+  void addStringOption(const string name, string & option_field, string default_value);
+
+  void addIntegerOption(const string name, int & option_field, int default_value);


Can you add documentation here, although there is a general explanation on top.
But it is not really the job of the 'refactorer' to sanitize everything, so you might as well ignore this comment ... and all my other comments 😄
As my brother tends to say: The 'good' shouldn't be the enemy of the 'better' ☮️

Most of the doxy stuff in these files only contribute to confusion, the methods explain themselves but then they have some copy pasted brief from other methods... I'll address some of your comments, thanks for looking through the stuff.

Common/src/config_structure.cpp

TobiKattmann · 2020-01-31T08:43:38Z

Common/include/config_structure.hpp

+
+  void addDoubleArrayOption(const string name, const int size, su2double * & option_field, su2double * default_value);
+
+  void addDoubleListOption(const string name, unsigned short & size, su2double * & option_field);


Here again no documentation

Common/include/config_structure.hpp

Common/include/option_structure.hpp

Common/include/option_structure.inl

disable config map declarations when not needed to speedup compilation

0c54dae

restructure and clean config_structure

pr-triage bot added the PR: unreviewed label Jan 29, 2020

pcarruscag changed the title ~~disable config map declarations when not needed to speedup compilation~~ Faster compilation Jan 29, 2020

pcarruscag added the enhancement label Jan 29, 2020

pcarruscag commented Jan 29, 2020

View reviewed changes

Merge branch 'develop' of github.com:su2code/SU2 into feature_faster_…

0ca9329

…compilation

Merge branch 'develop' into feature_faster_compilation

4ed133d

talbring reviewed Jan 29, 2020

View reviewed changes

SU2_DOT/include/SU2_DOT.hpp Outdated Show resolved Hide resolved

economon reviewed Jan 29, 2020

View reviewed changes

jayantmukho approved these changes Jan 31, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: unreviewed labels Jan 31, 2020

config_structure.X -> CConfig.X

66089bb

pr-triage bot added PR: unreviewed and removed PR: reviewed-approved labels Jan 31, 2020

talbring added the changelog:chore label Jan 31, 2020

talbring approved these changes Jan 31, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: unreviewed labels Jan 31, 2020

TobiKattmann reviewed Jan 31, 2020

View reviewed changes

fix some indentation issues

009a68c

pr-triage bot removed the PR: reviewed-approved label Jan 31, 2020

pr-triage bot added the PR: unreviewed label Jan 31, 2020

TobiKattmann approved these changes Jan 31, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: unreviewed labels Jan 31, 2020

pcarruscag merged commit b51b8cb into develop Jan 31, 2020

pcarruscag deleted the feature_faster_compilation branch January 31, 2020 12:21

pr-triage bot added PR: merged and removed PR: reviewed-approved labels Jan 31, 2020

pcarruscag mentioned this pull request Feb 6, 2020

Hybrid Parallel Compressible RANS Solvers #861

Merged

5 tasks

pcarruscag mentioned this pull request Mar 11, 2020

Hybrid parallel coloring fallback strategies (better strong scaling and user friendliness) #908

Merged

5 tasks

		@@ -174,34 +147,35 @@ enum AVERAGE_TYPE {
		AVERAGE_AREA = 1, /!< \brief Area-weighted average. /


		void addStringOption(const string name, string & option_field, string default_value);

		void addIntegerOption(const string name, int & option_field, int default_value);


		void addDoubleArrayOption(const string name, const int size, su2double * & option_field, su2double * default_value);

		void addDoubleListOption(const string name, unsigned short & size, su2double * & option_field);

Faster compilation #853

Faster compilation #853

Uh oh!

Conversation

pcarruscag commented Jan 29, 2020

Proposed Changes

PR Checklist

Uh oh!

pcarruscag Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

talbring commented Jan 29, 2020

Uh oh!

TobiKattmann commented Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

talbring left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pcarruscag commented Jan 29, 2020

Uh oh!

talbring commented Jan 29, 2020

Uh oh!

pcarruscag commented Jan 29, 2020

Uh oh!

economon commented Jan 29, 2020

Uh oh!

economon Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

pcarruscag Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

jayantmukho left a comment

Choose a reason for hiding this comment

Uh oh!

pcarruscag commented Jan 31, 2020

Uh oh!

talbring left a comment

Choose a reason for hiding this comment

Uh oh!

TobiKattmann left a comment

Choose a reason for hiding this comment

Uh oh!

TobiKattmann Jan 31, 2020

Choose a reason for hiding this comment

Uh oh!

pcarruscag Jan 31, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TobiKattmann Jan 31, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

TobiKattmann commented Jan 29, 2020 •

edited

Loading