firemodels / fds

Fire Dynamics Simulator
https://pages.nist.gov/fds-smv/
Other
659 stars 622 forks source link

OpenMP in mass.f90 #1226

Closed gforney closed 9 years ago

gforney commented 9 years ago
Please complete the following lines...

Application Version: latest
SVN Revision Number: 7098
Compile Date:
Operating System: various

Describe details of the issue below:

Hello,

This issue is (not quite) related to 1195; Christian R may be interested
in it.

I notice that the OpenMP code in mass.f90 (routines check_density()
and check_mass_fraction()) are disabled still (SVN 7098). I mentioned
on a post to the forum a while back that I guess this is to avoid
possible race conditions. I have now had a chance to have a slightly
closer look at what is going on.

In check_density(), the two main loops over i,j,k require
synchronisation of updates to rhodelta(:,:,:). This needs
a critical section

         IF (ISUM==0) THEN
            !$OMP CRITICAL
            RHODELTA(I,J,K) = RMIN - RHOP(I,J,K)
            !$OMP END CRITICAL
            CYCLE CHECK_LOOP
         ELSE
            IF(SUM-ISUM*RHO00 /= 0._EB) THEN
               !$OMP CRITICAL
               CONST = (RHOMIN-RHO00)/(SUM-ISUM*RHO00)
               IF (LC(-1)) THEN
.
.
.
               ENDIF
               RHODELTA(I,J,K) = RHODELTA(I,J,K) + RMIN - RHOP(I,J,K)
               !$OMP END CRITICAL
            ENDIF

and likewise in the second i,j,k loop. This appears to do the trick
for my test problem, which gives consistent answers for any number
of threads. (Test problem attached.)

In check_mass_fraction() the situation is a little more awkward. There does
appear to be a round-off issue here. For the test problem, you can see
this by, for example, reversing the order of the loops in the serial
code. This gives a slightly different answer in each case. This means
the OpenMP is always likely to give different answers on different
numbers of threads.

A rough-and-ready solution is to initialise yydelta to a non-zero
value (e.g, unity) and subtract the same value at the end of the
loop. This again gives consistent answers on different numbers of
threads. (You need to synchronise updates to yydelta as well, as above
for rhodelta.) However, it's not exactly the answer you got originally
with yydelta initialised to zero.

I assume the reason this action is required here and not in
check_density(), is that the values of the species fractions
can be widely varying in order of magnitude, while rho is
always roundabout the mean density. I don't know if you'd care
to adopt this as a permanent solution(!), but I can't see any
'low-impact' way to avoid the round-off at the moment. It at least
offers a way to run in OpenMP without having the appearance of a
race condition.

These changes are beneficial (or at least not harmful) to performance
in the test problem on hardware I have tried (Cray XT6). Hope this is
some help.

Cheers,
Kevin Stratford

Original issue reported on code.google.com by flexoelectric on 2010-11-08 12:19:57


gforney commented 9 years ago
Thanks for the input. I'll ask Christian to take a look at it.

Original issue reported on code.google.com by mcgratta on 2010-11-08 13:32:31

gforney commented 9 years ago
Thanks for the input. I will take a look. 

Original issue reported on code.google.com by crogsch on 2010-11-08 13:45:20

gforney commented 9 years ago
(No text was entered with this change)

Original issue reported on code.google.com by crogsch on 2010-11-08 13:46:02

gforney commented 9 years ago
What is the status of this issue?

Original issue reported on code.google.com by mcgratta on 2013-02-26 15:48:45

gforney commented 9 years ago
(No text was entered with this change)

Original issue reported on code.google.com by mcgratta on 2014-03-26 20:45:01

gforney commented 9 years ago
Both functions are no longer parallelised using OpenMP.

Testcase doesn't work with FDS6, Heptane is undefined.

Marking this as fixed.

Original issue reported on code.google.com by daniel.haarhoff on 2014-03-27 13:38:40

gforney commented 9 years ago
Thanks. Go ahead and mark the issue using one of the "Closed Statuses" options, like
"Verified" or "Closed" if you think that there is no need for confirmation from the
original poster. If you want the person to check if the fix works, use the "More Info"
status and write a request to the person to check.

Original issue reported on code.google.com by mcgratta on 2014-03-27 13:42:25

gforney commented 9 years ago
(No text was entered with this change)

Original issue reported on code.google.com by daniel.haarhoff on 2014-03-27 14:25:59