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

Restart DLA-Future and the pika runtime less frequently in C API tests #1268

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

msimberg
Copy link
Collaborator

@msimberg msimberg commented Feb 5, 2025

No description provided.

@msimberg
Copy link
Collaborator Author

msimberg commented Feb 5, 2025

cscs-ci run

@msimberg msimberg changed the title C api less start stop Start and stop the pika runtime less frequently in C API tests Feb 5, 2025
@msimberg msimberg force-pushed the c-api-less-start-stop branch from 69a883f to 8493b3f Compare February 5, 2025 17:18
@msimberg
Copy link
Collaborator Author

msimberg commented Feb 5, 2025

cscs-ci run

1 similar comment
@msimberg
Copy link
Collaborator Author

msimberg commented Feb 5, 2025

cscs-ci run

@msimberg msimberg force-pushed the c-api-less-start-stop branch from 26abeba to 2f154d0 Compare February 5, 2025 19:08
@msimberg
Copy link
Collaborator Author

msimberg commented Feb 5, 2025

cscs-ci run

@msimberg
Copy link
Collaborator Author

msimberg commented Feb 6, 2025

This does help quite a bit. In the last commit (26abeba) I only start and stop the runtime once per google-test. In the previous commit (70a69d9) I skip starting and stopping for the innermost loop but keep the others (it's still a good stress test, though not very realistic).

My proposal would be to drop the last commit so we start and stop a bit less often, but still fairly often (this already gives quite a nice speedup).

@msimberg
Copy link
Collaborator Author

msimberg commented Feb 6, 2025

Already requesting reviews, or at least comments, on this. I would still need to update the other C API tests, I've only applied the change to the gen_eigensolver one so far. Waiting for comments before applying changes elsewhere.

@msimberg
Copy link
Collaborator Author

msimberg commented Feb 7, 2025

I've rebased this on master instead of #1225, since it's not a requirement.

@RMeli
Copy link
Member

RMeli commented Feb 7, 2025

Starting/stopping once per test sounds good to me (26abeba). I think at some point we already used this approach to avoid a memory leak in pika (see #886 (comment)), but I don't recall why we decided to to start and stop the runtime at every test once the leak was fixed.

@msimberg msimberg force-pushed the c-api-less-start-stop branch from a000a10 to 53da3ae Compare February 10, 2025 09:00
@msimberg
Copy link
Collaborator Author

cscs-ci run

@msimberg msimberg changed the title Start and stop the pika runtime less frequently in C API tests Restart DLA-Future and the pika runtime less frequently in C API tests Feb 10, 2025
@msimberg
Copy link
Collaborator Author

Starting/stopping once per test sounds good to me (26abeba).

I've updated both eigensolver tests to restart DLAF/pika once per grid (we need the grid for init).

I think at some point we already used this approach to avoid a memory leak in pika (see #886 (comment)), but I don't recall why we decided to to start and stop the runtime at every test once the leak was fixed.

I don't remember for sure either, but I think it was to have a regression/unit test for the kind of issues that that mode exposed (memory leaks and other issues from restarting everything a lot of times), but I think we didn't consider the test time implications of it.

@msimberg msimberg marked this pull request as ready for review February 10, 2025 09:06
@rasolca rasolca merged commit 28bc43f into eth-cscs:master Feb 11, 2025
5 checks passed
@msimberg msimberg deleted the c-api-less-start-stop branch February 11, 2025 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants