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

Fix for some stack overflow issues with FAST.Farm #2452

Merged
merged 2 commits into from
Oct 21, 2024

Conversation

andrew-platt
Copy link
Collaborator

@andrew-platt andrew-platt commented Oct 8, 2024

Ready to merge

Feature or improvement description
We have been having issues with stack overflows in FAST.Farm compiled with Intel and Visual Studio. The root cause is due to copying of large wind grids within the AWAE module.

Using the zip files attached to issue #2053, this was eventually tracked down to line 1078 in AWAE.f90 which reads:

m%u_IfW_Low%PositionXYZ = p%Grid_low

The p%Grid_low is of unknown size at compile time, but can be extremely large (3x1643364 in the test case). This copy involves a temporary array and would normally be handled on the stack, but could result in an overflow for some models. By setting /heap-arrays:1000, any operation resulting in a temporary array of unknown array size at compile time will use the heap for the temprary array, and as will any array known at compile time to be larger than 1000 kB.

See https://www.intel.com/content/www/us/en/docs/fortran-compiler/developer-guide-reference/2024-2/heap-arrays.html for reference.

Related issue, if one exists
This solves #2053, and may also solve #843 and #2241

Impacted areas of the software
Certain models that had very large arrays that were being copied.

Additional supporting information
Ideally we would find a different way of handling the copy in AWAE.f90 line 1078 (maybe a pointer to the dataset or similar operation that doesn't require the copy at all -- I'm assuming this is a bottleneck).

Test results, if applicable
No test results are affected.

We have been having issues with stack overflows in FAST.Farm when large
wind grids were passed to AWAE.  This was eventually tracked down to
line 1078 in AWAE.f90 which reads:
`m%u_IfW_Low%PositionXYZ = p%Grid_low`
The `p%Grid_low` is of unknown size at compile time, but can be
extremely large (3x160000 or more).  This copy involves a temporary
array and would normally be handled on the stack, but could result in
an overflow for some models.  By setting the `/heap-arrays:1000` any
operation resulting in a temporary array of unknown array size at
compile time will use the heap for the temprary array, and as will any
array known at compile time to be larger than 1000 kB.

Testing shows that this fixes issue OpenFAST#2053, and will likely also solve OpenFAST#843 and OpenFAST#2241

See
https://www.intel.com/content/www/us/en/docs/fortran-compiler/developer-guide-reference/2024-2/heap-arrays.html
for reference.
@andrew-platt andrew-platt added this to the v3.5.4 milestone Oct 8, 2024
@andrew-platt andrew-platt self-assigned this Oct 8, 2024
@andrew-platt
Copy link
Collaborator Author

Notes:

  1. I am a little unsure if I have the syntax correct in the cmake file. The documentation linked above says the flag on windows should be /heap-arrays:1000, but the VS project produces the flag /heap-arrays1000. I suspect either should work, but don't have a good way to check at the moment.
  2. The 1 MB limit was arbitrarily chosen. This probably isn't an issue since the compiler will put any temporary arrays of unknown size at compile time on the heap instead of stack with this new flag.
  3. It may be worth adding this flag to the VS projects for drivers that may have large temporary arrays (I may add that before merging)

@andrew-platt
Copy link
Collaborator Author

@bjonkman, could you verify that this is a sensible solution?

@andrew-platt andrew-platt merged commit 69f8dff into OpenFAST:rc-3.5.4 Oct 21, 2024
31 of 57 checks passed
This was referenced Oct 21, 2024
@andrew-platt andrew-platt deleted the b/FF_stackOverflow branch November 1, 2024 20:15
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.

3 participants