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 !$ OMP critical around file opening for VTK #2340

Merged

Conversation

andrew-platt
Copy link
Collaborator

This is the preferred alternate approach to PR #2339. If this works, this PR will be merged in and the other closed.

Feature or improvement description
We were having problems with the !OMP directives around high resolution file reading in AWAE.f90. Since the file reading starts with a call to GetNewUnit before starting the opening, parallel calls to GetNewUnit could result in the same unit number handed out to two processes. The first process would open the file and start reading, but then the second process would open a different file with the same unit number causing read errors for both processes as they attempted to read the same file at the same time.

Adding $OMP critical around the GetNewUnit and following OpenFile... calls so that in theory these cannot be done in parallel.

Related issue, if one exists
#2339

Impacted areas of the software
VTK file reading within !$OMP loops

Additional supporting information

Test results, if applicable
@rthedin will report on testing with many turbines in a FAST.Farm simulation.

@andrew-platt andrew-platt added this to the v3.5.4 milestone Jul 19, 2024
@andrew-platt andrew-platt self-assigned this Jul 19, 2024
@andrew-platt andrew-platt force-pushed the b/OpenMP_FF_vtk_read_Opt2 branch 2 times, most recently from ed6bad7 to c2552ec Compare July 19, 2024 19:06
We were having problems with the `!OMP` directives around high resolution file reading in AWAE.f90. Since the file reading starts with a call to `GetNewUnit` before starting the opening, parallel calls to `GetNewUnit` could result in the same unit number handed out to two processes.  The first process would open the file and start reading, but then the second process would open a different file with the same unit number causing read errors for both processes as they attempted to read the same file at the same time.

Adding `$OMP critical` around the `GetNewUnit` and following `OpenFile...` calls so that in theory these cannot be done in parallel.

Co-authored-by: Derek Slaughter <deslaughter@gmail.com>
@rthedin
Copy link

rthedin commented Jul 19, 2024

Tested on Kestrel on an old 5-turbine case that worked with 3.5.3 commenting out the !OMP directives as Andy mentioned above. It runs with no problem.

The following header is printed:

 FAST.Farm-v2.5.0-3190-g0e7774a6
 Compile Info:
  - Compiler: Intel(R) Fortran Intel(R) 64 Compiler Classic for applications running on Intel(R)
  64, Version 2021.10.0 Build 20230609_000000
  - Architecture: 64 bit
  - Precision: double
  - OpenMP: Yes, number of threads: 104/104

It seems like there is an issue with the version number. Also, note that I have not noticed any performance difference between omp-commented-out 3.5.3 and this branch. CPU use patterns are the same, estimated time for completion is within 2 minutes of each other (total expected runtime ~6 hours).

Edit to add: Both runs with debug build.

@rthedin
Copy link

rthedin commented Jul 19, 2024

I don't have a test case ready with many (100+) turbines. I will test it, but might take a few days.

@andrew-platt
Copy link
Collaborator Author

With a debug run, there may not be a big performance increase given all the other single threaded parts of AWAE. Thanks for testing @rthedin!

@andrew-platt andrew-platt merged commit 44ff26d into OpenFAST:rc-3.5.4 Jul 19, 2024
19 checks passed
@andrew-platt andrew-platt deleted the b/OpenMP_FF_vtk_read_Opt2 branch July 19, 2024 21:17
@andrew-platt andrew-platt mentioned this pull request Oct 21, 2024
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants