OBC: segment parser refactoring #1200
Conversation
The segment configuration parser was encountering problems with GFortran 10.2, where the `fields` array update was being removed by the -O2 optimizer. In fact, the `parse_segment_data_str` function was doing two separate operations, where the first call would determine the number of fields and save their names into an array, and the second call would parse the data contents of each input field. The presence of optional arguments were used to effectively select the preferred operation. It is possible that the presence of these optional arguments was interfering with optimization, causing the removal of the `fields` update. While this is most likely a bug in the GFortran compiler, we address the problem by instead splitting this function into two independent functions, which allows us to remove the optional arguments. When split, the function appears to work correctly under O2 optimization.
|
Someday it might be better to rewrite this parser to populate a more organized data structure, but I think this change is good enough for now. |
|
Either this or another recent change is causing my seamount cases to fail: |
|
@kshedstrom Can you either send me the experiment or revert to dev/gfdl and test again? |
|
It is definitely dev/gfdl which is the problem. I'll go to an earlier version and try your patch. |
|
If I go to origin/dev/esmg and cherry-pick your patch, all is well. It does include Andrew's tides so maybe they are at odds with one of the other updates - I'll have to investigate further. I approve this PR. |
|
Thanks for checking Kate, there was a good chance I would make a mistake with this. |
|
@kshedstrom , @MJHarrison-GFDL we definitely need you to try this branch since we only have two tests that will exercise this code. |
|
@marshallward . Having trouble building (cd .testing; make TEMPLATE=./linux-ubuntu-xenial-gnu.mk compile). I was on the wrong branch. All is well. |
|
@kshedstrom . seamount cases pass with a clean build. |
|
I am OK with these changes. |
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #1200 +/- ##
============================================
- Coverage 46.08% 46.07% -0.01%
============================================
Files 214 224 +10
Lines 69399 70574 +1175
============================================
+ Hits 31984 32519 +535
- Misses 37415 38055 +640
Continue to review full report at Codecov.
|
The segment configuration parser was encountering problems with GFortran
10.2, where the
fieldsarray update was being removed by the -O2optimizer.
In fact, the
parse_segment_data_strfunction was doing two separateoperations, where the first call would determine the number of fields
and save their names into an array, and the second call would parse the
data contents of each input field. The presence of optional arguments
were used to effectively select the preferred operation.
It is possible that the presence of these optional arguments was
interfering with optimization, causing the removal of the
fieldsupdate.
While this is most likely a bug in the GFortran compiler, we address the
problem by instead splitting this function into two independent
functions, which allows us to remove the optional arguments.
When split, the function appears to work correctly under O2
optimization.