Adds NOOPT_FILES concept to mpas cmake system

[jgfouca]

And uses this system to deoptimize icepack_shortwave_data.F90, a file that takes a very long time to compile and is just setting up consts.

[BFB]

[github-actions]

PR Preview Action v1.4.8
🚀 Deployed preview to https://E3SM-Project.github.io/E3SM/pr-preview/pr-6668/
on branch gh-pages at 2024-10-07 17:42 UTC

[sarats]

Wuyin provided a pointer to a v3. HR run: https://pace.ornl.gov/exp-details/191652

While discussing v3 HR performance, we noticed the build times for that experiment: https://pace.ornl.gov/buildtime/191652

423.980000 Total_Elapsed_Time
200.788555 CMakeFiles/ice.dir/__/__/core_seaice/icepack/columnphysics/icepack_shortwave_data.f90.o

(This is almost half the time taken for the entire build)

[jonbob]

Approved based on visual inspection and testing -- it passed the e3sm_ice_developer suite on chrysalis

[eclare108213]

Icepack has a cpp flag to not compile the input tables. I don't think E3SM needs to compile those because it reads in the same data from files. See NO_SNICARHC in https://cice-consortium-icepack.readthedocs.io/en/main/user_guide/ug_case_settings.html#shortwave-nml

[sarats]

@eclare108213 If I understand your right, we can skip compilation of this file for E3SM build. Please confirm.
If there are specific configurations where this needs to be skipped, we need to see if that logic can be easily encapsulated in a CMake rule etc.

@jgfouca Once confirmed, can you remove this file from the CMake build sources then?

[eclare108213]

@sarats Now I'm not sure whether the cpp will work. In the E3SM Icepack slack channel where we are discussing this, @apcraig writes

Let me know if there is anything we can do on the Consortium side to make this better. The data is hardcoded in icepack_shortwave_data.F90. As @eclare indicates, you can hide the data with the NO_SNICARHC CPP if you don't need it. But I don't think we have a method to pass that data to Icepack and Icepack does not support any IO directly. We could add methods that allow a driver to read the data and pass it in if that were a better option.
Sounds like the slowdown comes from compiler optimization of the file. In fact, the long compile time is why we implemented the CPP. We didn't want to have to pay for the compile time for that method when running a test suite that mostly doesn't use it. So it's only "on" when snw_ssp_table='snicar'. I like the idea of adding it to a list of "no optimize" files. The compiler optimization almost certainly has no impact on the performance of those methods.

It sounds like we (CICE Consortium) only use the cpp for testing when we do not need those data values at all. You could try turning on the cpp locally and see if the answers change (or more likely that the code fails because it doesn't have the data it needs). If that happens, then definitely forge ahead with what you're doing now.

[rljacob]

Do you recall why all this data is hardcoded in a Fortran file instead of read in from a text file? Was it just to avoid doing any I/O ? ("Icepack does not support any IO directly.")

[eclare108213]

Do you recall why all this data is hardcoded in a Fortran file instead of read in from a text file? Was it just to avoid doing any I/O ? ("Icepack does not support any IO directly.")

Yes, that's right. E3SM is currently the only model using the 5-band snicar data, and we didn't want to have to maintain those data files just to support Icepack testing in the Consortium. E3SM in particular has wanted Icepack to be "lean", without all the bells and whistles available in CICE, and so we've resisted doing I/O. We can, though, if need be.

[jgfouca]

@sarats , if I remove this file, the build fails:

core_seaice/icepack/columnphysics/icepack_shortwave.f90:76:11:

   76 |       use icepack_shortwave_data, only: nspint_3bd, nspint_5bd
      |           1
Fatal Error: Cannot open module file 'icepack_shortwave_data.mod' for reading at (1): No such file or directory

Should I remove the icepack_shortwave.f90 as well?

[eclare108213]

No, you can't remove either icepack_shortwave.f90 or icepack_shortwave_data.f90. They are both needed for the radiation calculations, regardless of where the data is coming from. My suggestion was that you test whether MPAS-seaice is reading the data from data files and somehow sending that data to Icepack by activating the cpp NO_SNICARHC flag within the icepack_shortwave_data.F90 file, which only eliminates the enormous data tables. I don't think that will work, though.

So then the first question to ask is whether E3SM is reading in all that data from a file and then using the data from the hardcoded tables instead. If so, which do you prefer? Choose one or the other. They should be identical, at the moment. If you want to use the data files, then we'll need to generalize Icepack's snow_ssp_table to allow the data to come from somewhere else and we'll need to make sure that's BFB. If you want to use the data from the tables (which is what I believe it's doing now), then your NOOPT_FILES approach should work without any modification to Icepack.

[eclare108213]

We've had some discussion of this in the E3SM Icepack slack channel. @rljacob A big motivator for hardcoding the tables in Icepack, which I had forgotten, is that the current data files don't have all of the dimension and other information needed to use the data -- that was hard-coded in E3SM. This is a potentially dangerous situation that we weren't willing to accommodate in Icepack. Rather than creating new data files that would have to be maintained, distributed, documented, read in, etc., we chose to go with the hard-coded tables instead.

I believe E3SM is using the hardcoded table data (-DUSE_SNICARHC is set) but also reading it in from the files. It doesn't need to do both. If you try using the ifdef in icepack_shortwave_data.F90 to ignore the tables, the code will fail because the data read in from the files wouldn't be passed into Icepack -- we don't yet have those variables in the interface. Since E3SM is using the tables now, then until the files are fixed/updated I recommend changing E3SM to not read the data in from them, and using your NOOPT_FILES approach here to improve the compile time.

[jgfouca]

@eclare108213 , it appears like all that hardcoded data is required:
ERROR: (icepack_shortwave_init_snicar) ERROR: USE_SNICARHC CPP required

[jgfouca]

Hah, we commented at almost the exact same time. So, I think we are good to merge this?

[jgfouca]

Merged to next.

[apcraig]

Just curious, can someone share the compile time different from optimized and not optimized for this file. Is it like 10x, 100x, 1000x?

[apcraig]

Also, for future reference, you don't need the CPP "USE_SNICARHC". It has no effect. At some point "USE_SNICARHC" became the default and the updated CPP is "NO_SNICARHC". So the CPP now allows a user to hide the hardcoded tables if they aren't needed instead of turning them on if they are. Anyway, you should remove the "USE_SNICARHC" when you have a chance for clarity.

[rljacob]

@apcraig this comment has a "with optimization" time #6668 (comment).

[apcraig]

@apcraig this comment has a "with optimization" time #6668 (comment).

Is there also a time for the "without optimization". Just curious whether turning off the optimization has a large impact or whether some other aspects (like number of lines) is playing a large role too.

[jonbob]

@apcraig -- the switch to "NO_SNICARHC" may not have made it into E3SM yet. Looking at icepack_shortwave_data.F90 I still see:

#ifndef USE_SNICARHC
      character(len=*),parameter :: subname='(icepack_shortwave_init_snicar)'
      call icepack_warnings_setabort(.true.,__FILE__,__LINE__)
      call icepack_warnings_add(subname//' ERROR: USE_SNICARHC CPP required')
      return
#else

[jgfouca]

@apcraig , did you see my comment #6668 (comment) ?

USE_SNICARHC has an effect. At least in the version of mpassi we are using.

[eclare108213]

It looks like that's in the older version of Icepack that E3SM still has. We plan to resync them soon.

components/mpas-seaice/src/icepack/columnphysics/icepack_shortwave_data.F90:#ifndef` USE_SNICARHC
components/mpas-seaice/src/icepack/columnphysics/icepack_shortwave_data.F90:      call icepack_warnings_add(subname//' ERROR: USE_SNICARHC CPP required')

[apcraig]

Sorry about that, thought you had that change in your version. Yes, keep the USE_SNICARHC turned on for now.

[rljacob]

@jgfouca please start merging this.