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

Add an experimental profiling API #1657

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft

Conversation

fsimonis
Copy link
Member

@fsimonis fsimonis commented May 12, 2023

Main changes of this PR

This PR adds an experimental profiling API to preCICE.

In user/adapter-code, one can call pushProfilingSection("name") to add events and popProfilingSection() to stop the last event.
These events are marked as fundamental and scoped under solver.initialize/ or solver.advance/.

Motivation and additional information

This should simplify the integration of profiling for adapter and tools into the precice profiling.

Closes #1647

Author's checklist

  • I used the pre-commit hook to prevent dirty commits and used pre-commit run --all to format old commits.
  • I added a changelog file with make changelog if there are user-observable changes since the last release.
  • I added a test to cover the proposed changes in our test suite.
  • For breaking changes: I documented the changes in the appropriate porting guide.
  • I sticked to C++17 features.
  • I sticked to CMake version 3.16.3.
  • I squashed / am about to squash all commits that should be seen as one.

Reviewers' checklist

  • Does the changelog entry make sense? Is it formatted correctly?
  • Do you understand the code changes?

@fsimonis fsimonis added the enhancement A new feature, a new functionality of preCICE (from user perspective) label May 12, 2023
@uekerman
Copy link
Member

Is it clear for the user that with sync-mode="on", they could create their own deadlock?

In general, I am a bit afraid that we are opening another box for v3.0 🙈

@fsimonis
Copy link
Member Author

I don't think that user events should be synchronised and in this PR they aren't. This can always be done in user code.

@davidscn
Copy link
Member

Right now, I don't see the real use-case for these API functions. Main reasons:

  • the adapters speak the language of the solvers and if the solver is performance oriented, they will have their own performance instrumentation, which is also more natural to use then
  • for tools like ASTE, the interesting part happens in preCICE, not in ASTE
  • we already measure the solver.advance calls which are a summary of the solver events within a time-step

To me this API sound more like an extension of the framework we want to couple.

@fsimonis
Copy link
Member Author

fsimonis commented May 15, 2023

the adapters speak the language of the solvers and if the solver is performance oriented, they will have their own performance instrumentation, which is also more natural to use then

Yes in theory.
In practise, this doesn't seem to be straight forward though (FEniCS, OpenFOAM). Maybe we can get some input by other adapter devs here: @BenjaminRodenberg @IshaanDesai @MakisH @uekerman

A valuable use-case in my mind is allowing to profile adapters of commercial and/or closed-source codes such as Fluent @mtree22 or TAU.
Some tools like CalculiX also don't use profiling themselves.

for tools like ASTE, the interesting part happens in preCICE, not in ASTE

True.

However, it would remove the need for custom profiling inside ASTE.
Given that ASTE is basically pure IO and distributed filesystems tend to be slow, it may be interesting to profile the time spend creating output files for example. Especially as we use ASTE on big distributed machines.

Other tools could profit from this functionality, prime example being the micro-manager @IshaanDesai.

Hopefully I tagged enough people to get some input 😉

@uekerman
Copy link
Member

IMO, this is useful, but I would postpone it to 3.x.

@IshaanDesai
Copy link
Member

This API would be quite useful in the Micro Manager. But the names of the functions confused me a bit, wouldn't startProfiling("name") and stopProfiling() make more sense?

@fsimonis fsimonis added this to the Version 3.x.x milestone May 17, 2023
@fsimonis
Copy link
Member Author

fsimonis commented Jun 2, 2023

This will also work nicely with #1673

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature, a new functionality of preCICE (from user perspective)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Profiling of user-code via API
4 participants