Feature/optimize headers

[mgovers]

Part 1 of #486 :

• move some very common headers to a new common/ directory
  • I made the decision on which headers to move there based on the question: Is this something that makes sense in another context than just the high-performance power system simulation library context?
• remove some of the STL libraries loaded in the very common headers - include what you need.

unexpectedly, this did not improve the build speed. my assumption is that the most expensive part is actually the part where math model is constructed/used

however, this is still a nice improvement IMO, so i would still like to go ahead with this change. The rest is follow-up and will be done in a separate PR

[TonyXiang8787]

@mgovers what do you mean part 1 here? The first bullet point? Maybe just list the changes in the PR.

[mgovers]

@mgovers what do you mean part 1 here? The first bullet point? Maybe just list the changes in the PR.

i will when it goes out of draft. it's still in draft so i can't be bothered for the time being. I did, however, need CI to make sure that whatever changes i've done so far are correct

[mgovers]

all sonar cloud issues were already present on main

[Jerry-Jinfeng-Guo]

• Moving 'power_grid_model_.hpp' to 'common' is very confusing...
• Are we ever considering UUID's as preprocessor directive instead of having to maintain them like in this PR?
• As I do not know yet what it is like before the current grouping of headers, I think we can still re-organize a lot. For example, you would expect topology.hpp to live within ./core, but apparently there is another topology.hpp; parameters and containers headers looks also like they don't belong in the ./pgm-root directory; some resemblance/reference between headers (that do live in ./pgm-root dir) and directories would be much more logical to me.

[TonyXiang8787]

unexpectedly, this did not improve the build speed. my assumption is that the most expensive part is actually the part where math model is constructed/used

@mgovers In the CI you can see the timestamp of each individual compilation of translation unit. There you can have an idea where it took long time to compile.

[Jerry-Jinfeng-Guo]

I tried compiling this branch (e.g.) and it seems the test cases are taking up a significant portion of the total compilation time.
(model.cpp vs test-main-model.cpp)

[mgovers]

unexpectedly, this did not improve the build speed. my assumption is that the most expensive part is actually the part where math model is constructed/used

@mgovers In the CI you can see the timestamp of each individual compilation of translation unit. There you can have an idea where it took long time to compile.

yes, i know. i already know what the most probable issue is, but then i had already done these changes and do not want to do those in the same PR

[mgovers]

• Moving 'power_grid_model_.hpp' to 'common' is very confusing...

do you have a better name? IMO power_grid_model.hpp actually should contain everything of the PGM instead only the basic type declarations, meaning the name of the header is wrong, but i don't really have a better idea.

* Are we ever considering UUID's as preprocessor directive instead of having to maintain them like in this PR?

that's very compiler-specific and unfortunately won't solve our problem. I have looked into precompiled headers before but that makes stuff even worse cause it's a very slow process for large header files (including all include statements).

* As I do not know yet what it is like before the current grouping of headers, I think we can still re-organize a lot. For example, you would expect `topology.hpp` to live within `./core`, but apparently there is another `topology.hpp`; parameters and containers headers looks also like they don't belong in the ./pgm-root directory; some resemblance/reference between headers (that do live in ./pgm-root dir) and directories would be much more logical to me.

yeah, we can discuss that during the design discussion on the tap changer. this change just applies the changes in #486

[TonyXiang8787]

@mgovers @Jerry-Jinfeng-Guo, about header include guard, what do you think of remove the include guard entirely. Just use the #pragma once. This is not standard C++, but basically supported by all major compilers.

[Jerry-Jinfeng-Guo]

• Moving 'power_grid_model_.hpp' to 'common' is very confusing...

do you have a better name? IMO power_grid_model.hpp actually should contain everything of the PGM instead only the basic type declarations, meaning the name of the header is wrong, but i don't really have a better idea.

I would also strongly argue for that power_grid_model.hpp actually should contain (references to) everything. Therefore, common is not the best place for it to be? I think it should just stay under the include_root.

• Are we ever considering UUID's as preprocessor directive instead of having to maintain them like in this PR?

that's very compiler-specific and unfortunately won't solve our problem. I have looked into precompiled headers before but that makes stuff even worse cause it's a very slow process for large header files (including all include statements).

Hmm, interesting. Simply replacing the - with _ would make them just as easy as the current way. Perhaps you mean it's heavy on the reading side?

[mgovers]

@mgovers @Jerry-Jinfeng-Guo, about header include guard, what do you think of remove the include guard entirely. Just use the #pragma once. This is not standard C++, but basically supported by all major compilers.

I'm ok with it. it has some caveats but i don't think we run into those. In addition, declaring #pragma once before the header guard itself actually runs into the same caveats. I do believe it is widely enough supported to just go through with it. I can either do that in this PR or a separate one. What are your thoughts?

[mgovers]

• Moving 'power_grid_model_.hpp' to 'common' is very confusing...

do you have a better name? IMO power_grid_model.hpp actually should contain everything of the PGM instead only the basic type declarations, meaning the name of the header is wrong, but i don't really have a better idea.

I would also strongly argue for that power_grid_model.hpp actually should contain (references to) everything. Therefore, common is not the best place for it to be? I think it should just stay under the include_root.

Well, yes, that's what i mean 😬 therefore, the name of this file is wrong. What would be a better name?

• Are we ever considering UUID's as preprocessor directive instead of having to maintain them like in this PR?

that's very compiler-specific and unfortunately won't solve our problem. I have looked into precompiled headers before but that makes stuff even worse cause it's a very slow process for large header files (including all include statements).

Hmm, interesting. Simply replacing the - with _ would make them just as easy as the current way. Perhaps you mean it's heavy on the reading side?

maybe we're talking about effectively two different things. Which type of UUID preprocessor directive are you talking about? I was thinking about speeding up class compilation by prefixing the class with __declspec(uuidof(class name)) as in https://stackoverflow.com/questions/23977244/how-can-i-define-an-uuid-for-a-class-and-use-uuidof-in-the-same-way-for-g

[Jerry-Jinfeng-Guo]

@mgovers @Jerry-Jinfeng-Guo, about header include guard, what do you think of remove the include guard entirely. Just use the #pragma once. This is not standard C++, but basically supported by all major compilers.

I was under the impression ditching the former is the way to go... 😂 But they are indeed redundant. I was accustomed to the
#if defined(_MSC_VER) #pragma once #endif
way of semi-ditching the former but always keep the guards. If we are gonna make that descision, I think we need some feedback from the user side. Or we put the guards away under conditions.

[Jerry-Jinfeng-Guo]

maybe we're talking about effectively two different things. Which type of UUID preprocessor directive are you talking about? I was thinking about speeding up class compilation by prefixing the class with __declspec(uuidof(class name)) as in https://stackoverflow.com/questions/23977244/how-can-i-define-an-uuid-for-a-class-and-use-uuidof-in-the-same-way-for-g

Oh, I was refering to e.g.:
#ifndef POWER_GRID_MODEL_COMMON_POWER_GRID_MODEL_HPP #define POWER_GRID_MODEL_COMMON_POWER_GRID_MODEL_HPP ... #endif // POWER_GRID_MODEL_COMMON_POWER_GRID_MODEL_HPP
to
#ifndef UUID_74198658_bb0c_451a_b024_7b0eec07 #define UUID_74198658_bb0c_451a_b024_7b0eec07 ... #endf // UUID_74198658_bb0c_451a_b024_7b0eec07
BUT, given the discussion over the guards, I think this is no longer a necessary thing to discuss.

[Jerry-Jinfeng-Guo]

• Moving 'power_grid_model_.hpp' to 'common' is very confusing...

do you have a better name? IMO power_grid_model.hpp actually should contain everything of the PGM instead only the basic type declarations, meaning the name of the header is wrong, but i don't really have a better idea.

I would also strongly argue for that power_grid_model.hpp actually should contain (references to) everything. Therefore, common is not the best place for it to be? I think it should just stay under the include_root.

Well, yes, that's what i mean 😬 therefore, the name of this file is wrong. What would be a better name?

Well, since it lives inside common, why not common.hpp?

[TonyXiang8787]

Please remove header include guard (also the generated headers). Just use #pragma once.

[mgovers]

Please remove header include guard (also the generated headers). Just use #pragma once.

ok i was going to do that in a separate PR but i'll do it in this one instead

[sonarqubecloud]

Quality Gate passed

Issues
22 New issues

Measures
0 Security Hotspots
97.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud