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 names to omp critical statements #486

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

JanNiklasB
Copy link

Dear All,

with this pull request I want to add names to various #pragma omp critical statements.
If such a statement has no name, it blocks every other critical statement with no name, which may cause an increase in runtime.
For example, a shell output should only block other shell outputs and not file outputs.
Furthermore, if not accessed through a python script but directly through c++, critical statements from the users files may also block critical statements in CRPropa.

@lukasmerten
Copy link
Member

lukasmerten commented May 8, 2024

Hi @JanNiklasB
Thanks for contributing to CRPropa. I did not see any problems with your suggested improvement.
If you apply the two small updates suggested above to the code, I will approve it and merge afterwards.

Here, is a reference to the IBM description of the omp critical statement (IBM omp critical).

@@ -21,7 +21,7 @@ ParticleCollector::ParticleCollector(const std::size_t nBuffer, const bool clone
}

void ParticleCollector::process(Candidate *c) const {
#pragma omp critical
#pragma omp critical(ModifiyContainer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, is a typo

@@ -248,7 +248,7 @@ void HDF5Output::close() {
}

void HDF5Output::process(Candidate* candidate) const {
#pragma omp critical
#pragma omp critical(HDF5Output_access_file)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there is no other critical statement HDF output module, I would just go for "HDFOutput".

small typo and HDF5Output_access_file to HDFOutput
@JanNiklasB
Copy link
Author

Hey,
I applied the two small updates, now the typo is fixed and the name for the HDFOutput is more generalized.
Sorry that this took so long, I am new to github and did not saw or could not find the "two small updates" you wrote about
and forgot about it untill now.

@lukasmerten
Copy link
Member

Hi @JanNiklasB
That was not your fault. I missed to save my comments properly. They were showing up for me in the conversation but not for anyone else. So do not worry: Obviously you can make mistakes even after several years with GitHub.
So now everything looks fine from my sight. If I do not hear any objections, I will merge this in the next days.

@lukasmerten lukasmerten merged commit f5b2c43 into CRPropa:master Jun 3, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants