Pull request:
  https://github.com/aibasel/downward/pull/57~~ 
  https://github.com/aibasel/downward/pull/145

TODO:
* Update wiki docs.

We repeated runs for the eager-ff-config 10 times and saw that the results are not reliably reproducible:
https://ai.dmi.unibas.ch/_experiments/ai/downward/issue1028/data/issue1028-g++_sat_parking-eval/
Even repeated runs with the same search string and compiler settings were sometimes much faster and sometimes much slower. This configuration seems to just be very noisy. In the end, we decided to merge the changes, as we do not see any performance loss that we can reliably reproduce.

Some additional complication came from tests with g++-12, which is not available on the grid and caused our automated tests on Github to fail. One cause were uninitialized values in a struct, while the other cases was called by a compiler bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105651). We tried to work around it but eventually decided against this and ignored the warning (-Wno-restrict).

We also updated the Wiki with the new minimal and tested compiler versions, and adapted the Github workflow so it builds 
  - ubuntu-20.04 with gcc-10(*) and clang-12
  - ubuntu-22.04 with gcc-11, gcc-12(*), and clang-14
Where we use the version marked with (*) for exit-code tests, etc.

With that, I think we are ready to merge.

We repeated the experiments from msg10442 for c++11, c++17 and c++20 with g++ 10 and clang 12. Here are the results:

https://ai.dmi.unibas.ch/_experiments/ai/downward/issue1028/data/

We only checked the comparison between c++11 and c++20 so far. (We only wanted to look at c++17 if there are any issues with c++20.)
Memory never was an issue in any configuration, it always increases by a small factor for each instance (less than 100 KB in almost all cases, and we saw none higher than 1500). For time we looked at time scores overall and for each domain, and looked into the worst case for each configuration.

The most noticeable drop in performance happens for eager-ff with g++. Here we loose 5 tasks in coverage in domains with very large tasks (caldera, organic synthesis, satellite, parking). The instances are all lost because of running out of time on instances that took over 1000s to solve with c++11. In parking we saw on other instances that the time can differ by a large factor (up to 2). This was mostly in favor of c++11, but we also observed the opposite. The number of expansions stayed the same though in all cases that were solved by both.

In what follows we now report on all configurations other than eager-ff with g++.
Coverage stayed the same across the board. The worst total time score loss was -1.16 (loss was relatively equally distributed over many domains).
For all configurations we looked at the domain with the worst loss of time score and within the domain at the problem with the worst loss. For example for ipdb with g++ probfreecell-4-3 went from 149.34s to 172.74s. We tried to reproduce these results locally. For eager-ff with clang we only tried two runs, c++11 took 275.51s and 275.36s, while c++20 took 307.66s and 314.85s. For all other cases though the noise we got within the configuration was bigger than the difference between the configurations; if anything the c++20 version was faster.

Generally, the results seemed very noisy in both directions, more than the usual 5% noise we see when repeating an experiment.


We think that all configurations except eager-ff look like noise. We will look further into eager-ff, but not into any other config.

I started looking into compilers and supported features. Before rerunning the experiment and looking for the source of the slowdown, I wanted to see what compilers we should consider.

For Windows and Mac, the simplest solution would be to consider only the versions, we also test for on Github. This is fairly new and might exclude some older setups but I think on Mac at least it is common (or unavoidable) to always be on the latest version.

For Linux, I looked at what compilers are available in the previous LTS (20.04). I didn't look at the default, but at the latest version that is available through apt. With that, I got the following list of compilers:

  * g++-10
  * clang-12
  * MSVC 19.34.31937.0
  * AppleClang 13.0.0.13000029

I then looked at https://en.cppreference.com/w/cpp/compiler_support to find out which features this set of compilers supports. Attached, there is a spread sheet with the tables from that site. It is limited to the compilers we care about and color-coded for the set of compilers above: if support for a feature is only added in g++-11, the cell is colored red, for example.

With the set of compilers above, we'd get:
* all of C++-14
* all core features of C++-17
* most of the C++17 library features, including std::any and std::optional.
* a good chunk of the C++-20 core features, no modules but no compiler fully supports them yet.
* roughly half of the C++-20 library features

I think this is a good balance between using cool new language features and still having support for slightly older systems.

Next, I'd try re-run the experiment with g++-10 and clang-12. I have not yet checked if they are available in modules on the grid, but if not, I'm sure we could get a module for them.

We discussed in the Fast Downward meeting today to make some more effort on this, ideally going directly to C++20. Some preparatory steps, please comment to correct/extend.

1. Check if the compilers we want to support have sufficiently complete C++ support.

2. Related to this, update the information in our README which compiler versions we actually support. There is no need to support Ubuntu 18.04. For Ubuntu 20.04, I think it's enough if *some* compiler available in the standard repositories can compile the planner. It doesn't have to be the default compiler, and I think it's enough if it's only one of g++ and clang, but not both.

3. Check if the performance regression discussed here still exists, then decide what to do about it.

I agree that it would be good to understand what goes on in the cases you mention. However, I probably won't be able to look into this in more detail in the near future. If somebody else wants to take over here, please do :-)

I think the main reason why we don't use clang for experiments is because performance was worse than with g++ when we tested. It's worth retesting that every now and then when changing fundamental options. But I won't insist.

I think it would be good to merge, but I also think it would be good to use the opportunity to try to figure out what is going wrong in some of these domains. It's interesting that there is a significant slowdown in total time in some domains where there is no real effect on search time, both for iPDB and lama-first. (Didn't look in much more detail at other configurations.) There is a chance we're running into some subtle issues here that we didn't under older C++ rules, similarly to the pass by const reference performance issue we had recently.

For example, I would like to get at least an idea where we're using so much more time now in agricola for lama-first or in several domains (e.g. peg solitaire and PARCprinter) in ipdb.

I used g++ 8.3.0. I don't think an experiment with clang is so relevant since we don't usually use it for experiments. I think the benefit of using new language features (https://github.com/AnthonyCalandra/modern-cpp-features) outweighs the slight performance penalty, but I'm fine if you decide against merging this.

What do you suggest?
I assume this was with g++, what version?
Would it be interesting to also run an experiment with clang?

I ran some experiments. Here are the results:

https://www.ida.liu.se/~jense56/files/issue1028-v1-opt-issue1028-base-issue1028-v1-compare.html
https://www.ida.liu.se/~jense56/files/issue1028-v1-sat-issue1028-base-issue1028-v1-compare.html

Overall, the newer language version tends to increase runtime slightly.

I merged from main and added a changelog entry. Compilation works on all platforms now.

Regarding the TODO list: please also run experiments. New C++ standards enable new optimizations etc., and it's good to know what changes.

I added a pull request in issue1032.

When using -std=c++17, compilation fails for macOS because VariableOrderFinder uses std::random_shuffle(). In C++17 std::random_shuffle() needs to be given a UniformRandomBitGenerator, but I think we rather want to pass our own RNG and use its shuffle() method (there's a comment to this effect already). So the move to -std=c++17 is not so small after all and I think we should handle the VariableOrderFinder problem in a separate issue first. The class is only used by M&S and a pattern generator, your territory of the code, Silvan ;-) Would you care to look into this, Silvan?

We discussed upgrading to the C++17 language standard today and agreed that it would be nice to use the new language and library features.