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

Unit tests thermostats_test and random_test are very slow #3573

Closed
fweik opened this issue Mar 10, 2020 · 6 comments · Fixed by #3888 · May be fixed by #3884
Closed

Unit tests thermostats_test and random_test are very slow #3573

fweik opened this issue Mar 10, 2020 · 6 comments · Fixed by #3888 · May be fixed by #3884
Labels

Comments

@fweik
Copy link
Contributor

fweik commented Mar 10, 2020

in debug builds. This is due to the use of heap allocations and std::functions in the how loop.
With a better implementation these tests can be considerably sped up, which would lead to
a smoother workflow.
We should also discuss if statistical tests on the thermostats are needed or a good idea, or
if would be better if a mock RNG could be passed to then, and then checked if it is used correctly.

Also I'm not convinced that we should test the statistics of the RNG, I think it should rather be
tested that it implements philox correctly, e.g. checking that it produces the correct sequence
for a small set of parameters.

For reference, on my AMD Ryzen Threadripper 1920X, the tests take:

thermostats_test .................   Passed  191.93 sec
random_test ......................   Passed  147.47 sec
@fweik fweik added the Testcase label Mar 10, 2020
@jngrad
Copy link
Member

jngrad commented Mar 14, 2020

Concerning the runtime: I chose the number of samples to remain below the 15 sec mark in Debug builds. I found out today that Debug builds don't get their -Og flag overridden by -O0 when coverage is enabled, only the Release builds do. For Release buids with coverage, the runtime is between 80-300 sec depending on the hardware, which is too much.

@fweik
Copy link
Contributor Author

fweik commented Mar 16, 2020

Yes, this can also be easily fixed. E.g. replacing std::function and std::vector by just static constructs which can be deduced at compile time. I also don't think this is very high priority,
maybe a task for a coding day for somebody who wants to sharpen their C++ skills.

@jngrad
Copy link
Member

jngrad commented Mar 16, 2020

We need to weigh in on the importance of fixing those tests in the context of using -O0 vs. -Og for coverage builds, maybe during tomorrow's meeting. This thermostat test actually timed out in CI of #3582 when I replaced -Og by -O0 to be in line with recommendations from gcov. Looking through the git history, I couldn't find the rationale for using -Og. Was it to reduce test time in CI? Did we check if it had any impact on the coverage report?

@fweik
Copy link
Contributor Author

fweik commented Mar 16, 2020

Was it to reduce test time in CI? Did we check if it had any impact on the coverage report?

Yes I think so. That is basically turning on all the optimizations that are still compatible with coverage. I think -Og can be used everywhere if available?

@fweik
Copy link
Contributor Author

fweik commented Mar 16, 2020

Looking at the code, replacing the std::function is trivial. But my guess would be that most overhead is from the memory allocation caused by all the std::vectors that are created in destroyed in the hot-loop, in the call to noise_statistics. From the call sites I looked at neither the variant nor the heap allocations nor the std::function is needed, it's all static information.

@jngrad
Copy link
Member

jngrad commented Jun 7, 2020

Removing the std::function wrapper had no noticeable effect on timing (6216ab9). Using std::array instead of std::vector in the return value of lambdas reduced the random_test runtime by 15 s out of 4 min 44 s (cfd8156). I'm using a variant with a visitor pattern because the dimension of noise vectors depends on the RNG. Using instead a tuple wouldn't be convenient: since we cannot iterate over a tuple (std::get<i>(tuple) with i a variable won't compile), we would need something like a boost fusion or a hardcoded switch statement.

Depending on the unit test, between 30% and 50% of the runtime comes from the generation of random numbers.

@kodiakhq kodiakhq bot closed this as completed in #3888 Sep 18, 2020
kodiakhq bot added a commit that referenced this issue Sep 18, 2020
Description of changes:
- Remove RNG correlation stemming from seed offsets (fixes #3585)
    - seeds are now used as keys
    - a monotonically increasing counter is used in each thermostat
    - the only way to reset these counters is to create a new `System`
- Remove RNG correlation stemming from resetting `sim_time` or `time_step` during simulations with SD (fixes #3840)
    - the SD thermostat now uses the same RNG interface as other thermostats
- Accelerate RNG unit tests (fixes #3573)
    - they now take 2 seconds to run in coverage and sanitizer builds in CI
- Separate thermostats from integrators
    - better separation of concerns
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants