Carpet: Disable OpenMP parallelization of transport operators #3
Conversation
|
Most of this has been superseded by 5c036248cbb596ec17398faa4f1a3c26c3dd5b0b which is from August 2017. The non-superseded parts are collected in https://bitbucket.org/eschnett/carpet/branch/rhaas/no-openmp and amount to disabling OpenMP where what is done is basically a memory copy, as well as the change in timer name that I negatively commented on. Timer names are no longer truncated as of as git hash 6efd070 "TimerReport: allow arbirary long timer names" of cactusutils. Do you want to retire this pull request (or replace eschnett/no-openmp with rhaas/no-openmp) and apply only the leftover openmp parts? |
| timers.push_back(new Timers::Timer(name1.str())); | ||
| ostringstream name2; | ||
| name2 << "comm_state[" << timers.size() << "]" | ||
| << "." << tostring(state) << ".step"; | ||
| name2 << "comm_state[" << timers.size() << "].step." << tostring(state); |
There was a problem hiding this comment.
This seems to change the name of timers from "state.user" and "state.step" to "user.state" and "user.step" which is a non-backwards compatible, user visible change for anyone who wants to parse the output files for a particular user. Since the change is only cosmetic, I would rather keep the old names. This affects only the first of the two commits in the branch of the pull request.
There was a problem hiding this comment.
@eschnett says
This change tries to ensure that the important part of the timer name remains readable when the timer name is truncated, which happens in thorn TimerReport. Moving "user" and "step" (see below) to an earlier position in the string helps a lot.
There was a problem hiding this comment.
Wouldn't it then be better to remove the (arbitrary) restriction on timer name length from TimerReport? Eg use something better than (more or less) char [n_timers][100] timerbuffer?
I am aware this is very minor and will only affect code that parses for these timer names which is intrinsically "termporary" since it is performance analysis scripts and the like.
There was a problem hiding this comment.
See https://bitbucket.org/einsteintoolkit/tickets/issues/2268/timerreport-allow-arbirary-long-timer for a patch that removes the limit from TimerReport.
I’ll create version of this pull request that:
-
removes the omp parallel more cleanly
-
does not change the timer names
once that ticket has been reviewed and the pull request acted on.
| @@ -116,21 +116,20 @@ void copy_3d(T const *restrict const src, ivect3 const &restrict srcpadext, | |||
| ptrdiff_t const dstkoff = dstoff[2]; | |||
|
|
|||
| // Loop over region | |||
| if (use_openmp) { | |||
| if (false and use_openmp) { | |||
There was a problem hiding this comment.
this should not be committed in this form. Rather the whole "if" branch should be removed.
| @@ -101,13 +101,12 @@ void copy_4d(T const *restrict const src, ivect4 const &restrict srcpadext, | |||
| ptrdiff_t const dstloff = dstoff[3]; | |||
|
|
|||
| // Loop over region | |||
| if (use_openmp) { | |||
| if (false and use_openmp) { | |||
There was a problem hiding this comment.
this should not be committed in this form. Rather the whole "if" branch should be removed.
| // Loop over region | ||
| #pragma omp parallel | ||
| // Loop over region | ||
| // #pragma omp parallel |
There was a problem hiding this comment.
this should not be committed in this form. Rather the whole pragma should be removed.
| // Loop over region | ||
| #pragma omp parallel | ||
| // Loop over region | ||
| // #pragma omp parallel |
There was a problem hiding this comment.
this should not be committed in this form. Rather the whole pragma should be removed.
| // Loop over coarse region | ||
| #pragma omp parallel | ||
| // Loop over coarse region | ||
| // #pragma omp parallel |
There was a problem hiding this comment.
this should not be committed in this form. Rather the whole pragma should be removed.
| // Loop over coarse region | ||
| #pragma omp parallel | ||
| // Loop over coarse region | ||
| // #pragma omp parallel |
There was a problem hiding this comment.
this should not be committed in this form. Rather the whole pragma should be removed.
| // Loop over coarse region | ||
| #pragma omp parallel for collapse(3) | ||
| // Loop over coarse region | ||
| // #pragma omp parallel for collapse(3) |
There was a problem hiding this comment.
this should not be committed in this form. Rather the whole pragma should be removed.
| // Loop over coarse region | ||
| #pragma omp parallel | ||
| // Loop over coarse region | ||
| // #pragma omp parallel |
There was a problem hiding this comment.
this should not be committed in this form. Rather the whole pragma should be removed.
| // Loop over coarse region | ||
| #pragma omp parallel for collapse(4) | ||
| // Loop over coarse region | ||
| // #pragma omp parallel for collapse(4) |
There was a problem hiding this comment.
this should not be committed in this form. Rather the whole pragma should be removed.
@eschnett pull request 18 https://bitbucket.org/eschnett/carpet/pull-requests/18