Create the `provis_state` subpool at RK4 initialization to avoid memory leak #6334

sbrus89 · 2024-04-05T22:08:33Z

This PR fixes a memory leak I noticed in the RK4 timestepping when running 125 day single-layer barotropic tides cases with the vr45to5 mesh (MPAS-Dev/compass#802) on pm-cpu. Previously, I could only get through about 42 days of simulation before running out of memory.

This issue is related to creating/destroying the provis_state subpool at each timestep. We had a similar problem a few years back that required memory leaks fixes in the mpas_pool_destroy_pool (MPAS-Dev/MPAS-Model#367) subroutine. However, I believe there is still a memory leak in the mpas_pool_remove_subpool routine (which calls pool_remove_member) that is called following mpas_pool_destroy_pool. It seems like the TODO comment here:

E3SM/components/mpas-framework/src/framework/mpas_pool_routines.F

Lines 6036 to 6038 in 6b9ecaa

    
           !TODO: are there cases where we need to delete more data here? 
        
                                deallocate(ptr_prev) 
        
                             end if

suggests it is possible things aren't being completely cleaned up by this subroutine.

I'm not familiar enough with the low-level details of the pools framework to track down the memory leak itself. However, in any case, I think it makes more sense to create the provis_state subpool once at initialization as opposed to creating and destroying it every timestep. The main consequence of this approach is that the mpas_pool_copy_pool subroutine needs to have a overrideTimeLevel option similar to that used in mpas_pool_clone_pool used previously.

I've tested these changes with the vr45to5 tides test case and they do allow me to run for a full 125 days and are B4B with the previously create/destroy approach. The LTS and FB-LTS timestepping schemes are also updated here in the same way to prevent the same memory leak.

Since RK4 is not used in E3SM, this PR is B4B for all E3SM tests. The mpas_pool_copy_pool routine modified here is not used in MPAS-Seaice or MALI. See the Ocean-Discussions PR for additional background and testing.

[BFB]

- Avoids create/destroy at each timestep

sbrus89 · 2024-04-05T22:12:48Z

@matthewhoffman and @akturner, I thought I'd tag you on this since it touches the framework, even though this is apparently the first usage of mpas_pool_copy_pool in E3SM.

gcapodag · 2024-04-06T00:10:57Z

great, thanks @sbrus89

sbrus89 · 2024-04-08T19:58:40Z

@erinethomas and @darincomeau, would you mind taking a look at the framework changes in this PR just in case you see any issues from the sea ice side?

mark-petersen · 2024-04-10T21:21:56Z

Repeating post from E3SM-Ocean-Discussion#87 (comment)

Passes nightly test suite and compares bfb with master branch point on chicoma with optimized gnu and chrysalis with optimized intel. Also passes nighty test suite with debug gnu on chicoma. Note this includes a series of RK4 tests:

00:34 PASS ocean_global_ocean_Icos240_WOA23_RK4_performance_test
01:40 PASS ocean_global_ocean_Icos240_WOA23_RK4_restart_test
01:20 PASS ocean_global_ocean_Icos240_WOA23_RK4_decomp_test
01:20 PASS ocean_global_ocean_Icos240_WOA23_RK4_threads_test

In E3SM, passes

./create_test SMS_Ln9.T62_oQU240.GMPAS-IAF.chrysalis_gnu
./create_test SMS_Ln9.T62_oQU240.GMPAS-IAF.chrysalis_intel

mark-petersen · 2024-04-11T20:28:43Z

Repeating post from E3SM-Ocean-Discussion#87 (comment). Note these tests all do not use LTS.

Passes nightly test suite and compares bfb with master branch point on chicoma with optimized gnu and chrysalis with optimized intel. Also passes nighty test suite with debug gnu on chicoma. Note this includes a series of RK4 tests:

00:34 PASS ocean_global_ocean_Icos240_WOA23_RK4_performance_test
01:40 PASS ocean_global_ocean_Icos240_WOA23_RK4_restart_test
01:20 PASS ocean_global_ocean_Icos240_WOA23_RK4_decomp_test
01:20 PASS ocean_global_ocean_Icos240_WOA23_RK4_threads_test

In E3SM, passes

./create_test SMS_Ln9.T62_oQU240.GMPAS-IAF.chrysalis_gnu
./create_test SMS_Ln9.T62_oQU240.GMPAS-IAF.chrysalis_intel

mark-petersen

Tested with LTS on using gnu on chicoma and intel on chryslis, both debug and optimized. The following tests compare bfb against the master branchpoint on both.

ocean/dam_break/40cm/default_lts
  * step: initial_state
  * step: lts_regions
  * step: forward
  * step: viz
  test execution:      SUCCESS
  baseline comparison: PASS
  test runtime:        05:11
ocean/dam_break/120cm/default_lts
  * step: initial_state
  * step: lts_regions
  * step: forward
  * step: viz
  test execution:      SUCCESS
  baseline comparison: PASS
  test runtime:        01:00
ocean/dam_break/120cm/ramp_lts
  * step: initial_state
  * step: lts_regions
  * step: forward
  * step: viz
  test execution:      SUCCESS
  baseline comparison: PASS
  test runtime:        01:04

mark-petersen

The above tests show that we get the identical answer to master with standard time stepping and LTS. In addition, tests by @gcapodag show that this addition solves an out-of-memory error on perlmutter that he and @jeremy-lilly have been struggling with for the past two months. Thank you @sbrus89!

mark-petersen · 2024-04-11T20:45:14Z

@xylar and @matthewhoffman these changes are only used by the ocean core, and only for RK4 and LTS timestepping. I think you are safe with a visual review, as I tried to be thorough in my testing of RK4 and LTS and saw BFB results.

git grep -in mpas_pool_clone_pool
mpas-framework/src/core_sw/mpas_sw_time_integration.F:120:        call mpas_pool_clone_pool(statePool, provisStatePool, 1)
mpas-framework/src/framework/mpas_pool_routines.F:434:!  routine mpas_pool_clone_pool
mpas-framework/src/framework/mpas_pool_routines.F:444:   recursive subroutine mpas_pool_clone_pool(srcPool, destPool, overrideTimeLevels)!{{{
mpas-framework/src/framework/mpas_pool_routines.F:466:            call pool_mesg('ERROR in mpas_pool_clone_pool: Input time levels cannot be less than 1.')
mpas-framework/src/framework/mpas_pool_routines.F:909:                call mpas_pool_clone_pool(ptr % data % p, newmem % data % p)
mpas-framework/src/framework/mpas_pool_routines.F:921:   end subroutine mpas_pool_clone_pool!}}}
mpas-framework/src/framework/mpas_stream_manager.F:411:           call mpas_pool_clone_pool(manager % defaultAtts, new_stream % att_pool)
mpas-ocean/src/mode_forward/mpas_ocn_time_integration_lts.F:259:    call mpas_pool_clone_pool(tendPool, tendSum1stPool, 1)
mpas-ocean/src/mode_forward/mpas_ocn_time_integration_lts.F:261:    call mpas_pool_clone_pool(tendPool, tendSum2ndPool, 1)
mpas-ocean/src/mode_forward/mpas_ocn_time_integration_lts.F:263:    call mpas_pool_clone_pool(tendPool, tendSum3rdPool, 1)
mpas-ocean/src/mode_forward/mpas_ocn_time_integration_lts.F:265:    call mpas_pool_clone_pool(tendPool, tendSlowPool, 1)
mpas-ocean/src/mode_forward/mpas_ocn_time_integration_rk4.F:233:         call mpas_pool_clone_pool(statePool, provisStatePool, 1)

erinethomas · 2024-04-11T20:53:04Z

in line with @mark-petersen's comment above - I was not able to see any place where this would impact MPAS-SI this morning ... so I think its ok on the sea-ice side...

sbrus89 · 2024-04-11T21:00:23Z

@mark-petersen, the mpas-ocean rk4, lts, and fblts time stepping routines are the only places that use the mpas_pool_copy_pool routine I modified:

git grep -in mpas_pool_copy_pool
components/mpas-framework/src/framework/mpas_pool_routines.F:925:!  routine mpas_pool_copy_pool
components/mpas-framework/src/framework/mpas_pool_routines.F:935:   recursive subroutine mpas_pool_copy_pool(srcPool, destPool, overrideTimeLevels)!{{{
components/mpas-framework/src/framework/mpas_pool_routines.F:958:            call pool_mesg('ERROR in mpas_pool_copy_pool: Input time levels cannot be less than 1.')
components/mpas-framework/src/framework/mpas_pool_routines.F:1171:                   call mpas_pool_copy_pool(ptr % data % p, mem % p)
components/mpas-framework/src/framework/mpas_pool_routines.F:1181:   end subroutine mpas_pool_copy_pool!}}}
components/mpas-ocean/src/mode_forward/mpas_ocn_time_integration_fblts.F:230:      call mpas_pool_copy_pool(tendPool, tendSum3rdPool, 1)
components/mpas-ocean/src/mode_forward/mpas_ocn_time_integration_fblts.F:231:      call mpas_pool_copy_pool(tendPool, tendSlowPool, 1)
components/mpas-ocean/src/mode_forward/mpas_ocn_time_integration_lts.F:264:    call mpas_pool_copy_pool(tendPool, tendSum1stPool, 1)
components/mpas-ocean/src/mode_forward/mpas_ocn_time_integration_lts.F:265:    call mpas_pool_copy_pool(tendPool, tendSum2ndPool, 1)
components/mpas-ocean/src/mode_forward/mpas_ocn_time_integration_lts.F:266:    call mpas_pool_copy_pool(tendPool, tendSum3rdPool, 1)
components/mpas-ocean/src/mode_forward/mpas_ocn_time_integration_lts.F:267:    call mpas_pool_copy_pool(tendPool, tendSlowPool, 1)
components/mpas-ocean/src/mode_forward/mpas_ocn_time_integration_rk4.F:233:         call mpas_pool_copy_pool(statePool, provisStatePool, 1)

xylar

I looked over the code and discussed improvements with @sbrus89, which he implemented. I'm happy with the changes now. I agree that the framework changes only affect MPAS-Ocean and only in configurations that are not used in E3SM production simulations.

matthewhoffman

@sbrus89 , I'm not very familiar with this code, so what you've done to solve the problem makes sense to me, and I can't review it very critically. I have two small items, but they are not major concerns.

components/mpas-framework/src/framework/mpas_pool_routines.F

Create the provis_state subpool at RK4 initialization to avoid memory leak This PR fixes a memory leak in the RK4 timestepping when running 125 day single-layer barotropic tides cases with the vr45to5 mesh on pm-cpu. Previously, it could only get through about 42 days of simulation before running out of memory. This issue is related to creating/destroying the provis_state subpool at each timestep. Since RK4 is not used in E3SM, this PR is B4B for all E3SM tests. The mpas_pool_copy_pool routine modified here is not used in MPAS-Seaice or MALI. [BFB]

jonbob · 2024-04-25T20:15:53Z

passes:

ERS.ne4pg2_oQU480.WCYCL1850NS.chrysalis_intel
SMS_D_Ld1.ne30pg2_r05_IcoswISC30E3r5.CRYO1850-DISMF.chrysalis_intel

merged to next

jonbob · 2024-04-26T17:06:59Z

merged to master

This merge updates the E3SM-Project submodule from [93e511d](https://github.com/E3SM-Project/E3SM/tree/93e511d) to [31e0924](https://github.com/E3SM-Project/E3SM/tree/31e0924). This update includes the following MPAS-Ocean and MPAS-Frameworks PRs (check mark indicates bit-for-bit with previous PR in the list): - [ ] (ocn) E3SM-Project/E3SM#6256 - [ ] (ocn) E3SM-Project/E3SM#6224 - [ ] (ocn) E3SM-Project/E3SM#6270 - [ ] (ocn) E3SM-Project/E3SM#6293 - [ ] (ocn) E3SM-Project/E3SM#6321 - [ ] (ocn) E3SM-Project/E3SM#6262 - [ ] (ocn) E3SM-Project/E3SM#6300 - [ ] (ocn) E3SM-Project/E3SM#6334 - [ ] (ocn) E3SM-Project/E3SM#6371 - [ ] (ocn) E3SM-Project/E3SM#6288

sbrus89 added 3 commits April 2, 2024 12:52

Add provis_state subpool at RK4 init

c80ebd3

- Avoids create/destroy at each timestep

Improve overrideTimeLevels option for mpas_pool_copy_pool

12368b9

Add addtional subpools for LTS and FBLTS at init

2f49d49

sbrus89 added mpas-ocean bug fix PR BFB PR leaves answers BFB MPAS-Ocean standalone Issues and features for standalone MPAS-Ocean code that dont impact E3SM. labels Apr 5, 2024

sbrus89 requested review from mark-petersen, xylar and gcapodag April 5, 2024 22:08

sbrus89 assigned jonbob Apr 5, 2024

sbrus89 requested review from matthewhoffman and akturner April 5, 2024 22:10

mark-petersen removed the request for review from gcapodag April 10, 2024 21:16

mark-petersen reviewed Apr 11, 2024

View reviewed changes

mark-petersen approved these changes Apr 11, 2024

View reviewed changes

sbrus89 removed the request for review from akturner April 11, 2024 20:58

xylar approved these changes Apr 12, 2024

View reviewed changes

matthewhoffman reviewed Apr 16, 2024

View reviewed changes

components/mpas-framework/src/framework/mpas_pool_routines.F Show resolved Hide resolved

components/mpas-framework/src/framework/mpas_pool_routines.F Show resolved Hide resolved

Add comment explaining overrideTimeLevels

6f1a75d

matthewhoffman self-requested a review April 24, 2024 02:11

matthewhoffman approved these changes Apr 24, 2024

View reviewed changes

jonbob merged commit 6b38b86 into E3SM-Project:master Apr 26, 2024
12 checks passed

xylar mentioned this pull request May 8, 2024

Update E3SM-Project submodule MPAS-Dev/compass#821

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create the `provis_state` subpool at RK4 initialization to avoid memory leak #6334

Create the `provis_state` subpool at RK4 initialization to avoid memory leak #6334

sbrus89 commented Apr 5, 2024

sbrus89 commented Apr 5, 2024

gcapodag commented Apr 6, 2024

sbrus89 commented Apr 8, 2024

mark-petersen commented Apr 10, 2024

mark-petersen commented Apr 11, 2024

mark-petersen left a comment

mark-petersen left a comment

mark-petersen commented Apr 11, 2024

erinethomas commented Apr 11, 2024

sbrus89 commented Apr 11, 2024

xylar left a comment

matthewhoffman left a comment

jonbob commented Apr 25, 2024

jonbob commented Apr 26, 2024

	!TODO: are there cases where we need to delete more data here?
	deallocate(ptr_prev)
	end if

Create the provis_state subpool at RK4 initialization to avoid memory leak #6334

Create the provis_state subpool at RK4 initialization to avoid memory leak #6334

Conversation

sbrus89 commented Apr 5, 2024

sbrus89 commented Apr 5, 2024

gcapodag commented Apr 6, 2024

sbrus89 commented Apr 8, 2024

mark-petersen commented Apr 10, 2024

mark-petersen commented Apr 11, 2024

mark-petersen left a comment

Choose a reason for hiding this comment

mark-petersen left a comment

Choose a reason for hiding this comment

mark-petersen commented Apr 11, 2024

erinethomas commented Apr 11, 2024

sbrus89 commented Apr 11, 2024

xylar left a comment

Choose a reason for hiding this comment

matthewhoffman left a comment

Choose a reason for hiding this comment

jonbob commented Apr 25, 2024

jonbob commented Apr 26, 2024

Create the `provis_state` subpool at RK4 initialization to avoid memory leak #6334

Create the `provis_state` subpool at RK4 initialization to avoid memory leak #6334