Memory leak fix: pointers in interpolator and horiz_interp #1100

laurenchilutti · 2023-01-03T13:29:32Z

Description
In interpolator and horiz_interp, some pointers are being allocated. This PR changes the type from pointer to allocatable. Any use of if associated() has now been changed to if allocated() to accompany this change. There are also instances in the code where we were declaring, for example:

interface assignment(=)
  module procedure horiz_interp_type_eq
end interface

And in that subroutine horiz_interp_type_eq, for example:

horiz_interp_out%faci => horiz_interp_in%faci

I have changed these pointer assignment operators to an equals, with the foresight that this may cause issues. With the old functionality, anytime horiz_interp_out is modified, the value of horiz_interp_in is also modified. With my changes this will not be the case. I tested with the land model and did not run into any issues with this but we will test all models with alpha2 testing.

Fixes #1099

How Has This Been Tested?
Compiled with GNU and Intel, ran with the land model and answers reproduce

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules
New check tests, if applicable, are included
make distcheck passes

…types to allocatable type

laurenchilutti · 2023-01-03T13:30:34Z

@rem1776 Could @ganganoaa be added as a reviewer for this PR? Thanks

horiz_interp/horiz_interp_type.F90

thomas-robinson · 2023-01-03T14:10:05Z

horiz_interp/horiz_interp_type.F90

+    horiz_interp_out%wtj             =  horiz_interp_in%wtj
+    horiz_interp_out%i_lon           =  horiz_interp_in%i_lon
+    horiz_interp_out%j_lat           =  horiz_interp_in%j_lat
+    horiz_interp_out%src_dist        =  horiz_interp_in%src_dist
    horiz_interp_out%found_neighbors => horiz_interp_in%found_neighbors


Did you miss this pointer?

I couln't find any use of this pointer in FMS. Specifically if you grep the repository, you only get the following lines:

FMS/horiz_interp/horiz_interp_type.F90

Line 72 in 203c8bf

logical, dimension(:,:), pointer :: found_neighbors =>NULL() !< indicate whether destination grid

FMS/horiz_interp/horiz_interp_type.F90

Line 203 in 203c8bf

horiz_interp_out%found_neighbors => horiz_interp_in%found_neighbors

So, since it wasn't a pointer being allocated I did not touch that variable at all.

Does my reasoning make sense?

It seems odd because the routine is creating a copy except for this one variable.

I feel a little uncomfortable doing anything with the found_neighbors pointer. I have grepped the entire FMS repository for any use of found_neighbors and the only two times I can find any use of this is:

horiz_interp/horiz_interp_type.F90:72: logical, dimension(:,:), pointer :: found_neighbors =>NULL() !< indicate whether destination grid
horiz_interp/horiz_interp_type.F90:204: horiz_interp_out%found_neighbors => horiz_interp_in%found_neighbors

Since I don't fully understand what this variable is being used for, I would rather not modify it.

I'd bet its just unused in general, you don't get warnings for unused fields in derived types so it could have never been used and no one noticed. I think we can probably remove it unless we can find some code that actually uses it.

If we remove this field assignment, it will violate principle of object-to-object copy but if it is never used, we can remove. Instead, we can add something like this:

if (associated(in%var))
if (associated(out%var)) deallocate(out%var)
allocate(out%var(size(in%var))
out%var = in%var
else
out%var => NULL()
end if

@thomas-robinson @ganganoaa @rem1776 i have not done anything to found neighbors yet. the options as I see it are:

removing it seems like something that would require a lot of testing since its possible some model somewhere is using it.

changing it from pointer to allocatable change this pointer assignment to equals

leave it as is
I don't feel like I have enough familiarity with interpolator code to make this decision. How should I proceed? Who can I consult on what to do?

do this:

if (allocated(horiz_interp_in%found_neighbors))horiz_interp_out%found_neighbors = horiz_interp_in%found_neighbors

and make found_neighbors allocatable instead of a pointer. This is a = routine, so it should be making a copy, not providing pointers.

This is a modernization PR, not a clean up. No need to remove it here. We should note it for potential clean up in a future release.

ganganoaa · 2023-01-03T15:13:15Z

Yes. That was missed. horiz_interp_out%found_neighbors => horiz_interp_in%found_neighbors Ganga

…

On 1/3/23 09:15, Tom Robinson wrote: ***@***.**** requested changes on this pull request. ------------------------------------------------------------------------ In horiz_interp/horiz_interp_type.F90 <#1100 (comment)>: > @@ -189,35 +189,35 @@ subroutine horiz_interp_type_eq(horiz_interp_out, horiz_interp_in) call mpp_error(FATAL,'horiz_interp_type_eq: horiz_interp_type variable on right hand side is unassigned') endif - horiz_interp_out%faci => horiz_interp_in%faci The functionality of this routine is changing. Can you add doxygen comments to this clearly stating that this routine creates a /copy/ of the object. Also doxygen comments for the variables. ------------------------------------------------------------------------ In horiz_interp/horiz_interp_type.F90 <#1100 (comment)>: > - horiz_interp_out%wti => horiz_interp_in%wti - horiz_interp_out%wtj => horiz_interp_in%wtj - horiz_interp_out%i_lon => horiz_interp_in%i_lon - horiz_interp_out%j_lat => horiz_interp_in%j_lat - horiz_interp_out%src_dist => horiz_interp_in%src_dist + horiz_interp_out%faci = horiz_interp_in%faci + horiz_interp_out%facj = horiz_interp_in%facj + horiz_interp_out%ilon = horiz_interp_in%ilon + horiz_interp_out%jlat = horiz_interp_in%jlat + horiz_interp_out%area_src = horiz_interp_in%area_src + horiz_interp_out%area_dst = horiz_interp_in%area_dst + horiz_interp_out%wti = horiz_interp_in%wti + horiz_interp_out%wtj = horiz_interp_in%wtj + horiz_interp_out%i_lon = horiz_interp_in%i_lon + horiz_interp_out%j_lat = horiz_interp_in%j_lat + horiz_interp_out%src_dist = horiz_interp_in%src_dist horiz_interp_out%found_neighbors => horiz_interp_in%found_neighbors Did you miss this pointer? — Reply to this email directly, view it on GitHub <#1100 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A43PSQH5HBRREIEUXKGZHJ3WQQYA5ANCNFSM6AAAAAATPWEFNQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>

ganganoaa · 2023-01-04T15:41:36Z

I think it is safe since it is not allocated any where.

…

On 1/4/23 10:26, laurenchilutti wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In horiz_interp/horiz_interp_type.F90 <#1100 (comment)>: > @@ -189,35 +189,35 @@ subroutine horiz_interp_type_eq(horiz_interp_out, horiz_interp_in) call mpp_error(FATAL,'horiz_interp_type_eq: horiz_interp_type variable on right hand side is unassigned') endif - horiz_interp_out%faci => horiz_interp_in%faci Let me know if these changes are sufficient — Reply to this email directly, view it on GitHub <#1100 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A43PSQDBUJJA65UCFG3TCMLWQWJBXANCNFSM6AAAAAATPWEFNQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>

horiz_interp/horiz_interp_type.F90

thomas-robinson

I think you need to add if allocated statements and change that last pointer to an allocatable unless it's being used somewhere else as a pointer.

ganganoaa · 2023-01-11T15:58:54Z

Lauren, the fortran reference verified my assumption and what I have tested recently. Thanks.

…

On 1/11/23 10:38 AM, laurenchilutti wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In horiz_interp/horiz_interp_type.F90 <#1100 (comment)>: > + horiz_interp_out%facj = horiz_interp_in%facj + horiz_interp_out%ilon = horiz_interp_in%ilon + horiz_interp_out%jlat = horiz_interp_in%jlat + horiz_interp_out%area_src = horiz_interp_in%area_src + horiz_interp_out%area_dst = horiz_interp_in%area_dst + horiz_interp_out%wti = horiz_interp_in%wti + horiz_interp_out%wtj = horiz_interp_in%wtj + horiz_interp_out%i_lon = horiz_interp_in%i_lon + horiz_interp_out%j_lat = horiz_interp_in%j_lat + horiz_interp_out%src_dist = horiz_interp_in%src_dist @thomas-robinson <https://github.com/thomas-robinson> I wrote a test program to see if we would get an error if |horiz_interp_type_out| is unallocated, and there is no error and the program runs fine. I found a section of the Fortran 2003 standard that seems to indicate that If a member of horiz_interp_out is unallocated, it will be allocated with the same shape as horiz_interp_in. You can see the section of the fortran 2003 standard I am referring to here <https://wg5-fortran.org/N1601-N1650/N1601.pdf#page=155> in Section 7.4.1.3 Interpretation of intrinsic assignments — Reply to this email directly, view it on GitHub <#1100 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A43PSQGLLCA6QDXK4XTMXPLWR3HXZANCNFSM6AAAAAATPWEFNQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>

laurenchilutti · 2023-01-18T14:39:51Z

I have updated the found_neighbors field to be allocatable.
Lines 189-191 in hoiz_interp_type.F90 are checking that horiz_interp_in%I_am_initialized is set to .TRUE. %I_am_initialized is set to .TRUE. when these fields are allocated, so I will not be adding an if(allocated(in%field)) before each field assignment in horiz_interp_type_eq.
I added if (allocated(horiz_interp_in%found_neighbors)) before we equate the found_neighbors field because this is not allocated in FMS.

rem1776 · 2023-01-19T20:17:42Z

@laurenchilutti It looks like the CI glitched out or something on the last commit. I can see that it ran and passed yesterday here but the status never updated on the actual PR. I tried rerunning the latest jobs but it still didn't update.

I've seen something similar before it might've been a github action outage or something at the time you pushed it, so a new commit could fix it. Maybe you can revert/rebase and then re-push the last commit? Although it might be easier to just make some kind of small change and commit that.

laurenchilutti · 2023-01-20T13:32:36Z

@rem1776 I just added a minor change to a comment in the latest commit to relaunch the CI.

thomas-robinson

You need to check that all of the variables are allocated in the assignment routine. If they aren't allocated, I think that routine will crash.

horiz_interp/horiz_interp_type.F90

ganganoaa · 2023-01-20T15:47:49Z

I think the check before the assignments, if(.not.horiz_interp_in%I_am_initialized) then callmpp_error(FATAL,'horiz_interp_type_eq: horiz_interp_type variable on right hand side is unassigned') endif will suffice the requirement. If needed, extra safe-guard can be inserted. Ganga

…

On 1/20/23 10:15 AM, Tom Robinson wrote: ***@***.**** requested changes on this pull request. You need to check that all of the variables are allocated in the assignment routine. If they aren't allocated, I think that routine will crash. ------------------------------------------------------------------------ In horiz_interp/horiz_interp_type.F90 <#1100 (comment)>: > + horiz_interp_out%facj = horiz_interp_in%facj + horiz_interp_out%ilon = horiz_interp_in%ilon + horiz_interp_out%jlat = horiz_interp_in%jlat + horiz_interp_out%area_src = horiz_interp_in%area_src + horiz_interp_out%area_dst = horiz_interp_in%area_dst + horiz_interp_out%wti = horiz_interp_in%wti + horiz_interp_out%wtj = horiz_interp_in%wtj + horiz_interp_out%i_lon = horiz_interp_in%i_lon + horiz_interp_out%j_lat = horiz_interp_in%j_lat + horiz_interp_out%src_dist = horiz_interp_in%src_dist You need to do this for all of the allocatable variables. — Reply to this email directly, view it on GitHub <#1100 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A43PSQBS2WZDTNDOWBYX3B3WTKT2FANCNFSM6AAAAAATPWEFNQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>

thomas-robinson

Ignore my last comment because it is taken care of already.

laurenchilutti added 2 commits December 27, 2022 12:41

updating interpolator and horiz_interp from use of allocated pointer …

449ad13

…types to allocatable type

Merge branch 'main' into pointers

fdeb59f

laurenchilutti requested review from thomas-robinson, bensonr and rem1776 as code owners January 3, 2023 13:29

thomas-robinson requested changes Jan 3, 2023

View reviewed changes

thomas-robinson requested a review from ganganoaa January 3, 2023 14:15

ganganoaa requested a review from thomas-robinson January 3, 2023 16:14

Adding doxygen comments to horiz_interp_type_eq

42e061f

ganganoaa reviewed Jan 4, 2023

View reviewed changes

horiz_interp/horiz_interp_type.F90 Show resolved Hide resolved

thomas-robinson reviewed Jan 5, 2023

View reviewed changes

horiz_interp/horiz_interp_type.F90 Show resolved Hide resolved

thomas-robinson requested changes Jan 5, 2023

View reviewed changes

updating found_neighbors field of horiz_interp_type to alllocatable

baa7ba0

rem1776 previously approved these changes Jan 18, 2023

View reviewed changes

rem1776 mentioned this pull request Jan 19, 2023

Removing unused arguments throughout FMS #820

Open

updating comments

e81d4d8

laurenchilutti dismissed rem1776’s stale review via e81d4d8 January 20, 2023 13:31

thomas-robinson requested changes Jan 20, 2023

View reviewed changes

horiz_interp/horiz_interp_type.F90 Show resolved Hide resolved

thomas-robinson approved these changes Jan 20, 2023

View reviewed changes

rem1776 merged commit 91e7324 into NOAA-GFDL:main Jan 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory leak fix: pointers in interpolator and horiz_interp #1100

Memory leak fix: pointers in interpolator and horiz_interp #1100

laurenchilutti commented Jan 3, 2023

laurenchilutti commented Jan 3, 2023

thomas-robinson Jan 3, 2023

laurenchilutti Jan 3, 2023

laurenchilutti Jan 4, 2023

thomas-robinson Jan 5, 2023

laurenchilutti Jan 5, 2023

rem1776 Jan 17, 2023

ganganoaa Jan 17, 2023 •

edited

Loading

laurenchilutti Jan 18, 2023

thomas-robinson Jan 18, 2023

ganganoaa commented Jan 3, 2023 via email

ganganoaa commented Jan 4, 2023 via email

thomas-robinson left a comment

ganganoaa commented Jan 11, 2023 via email

laurenchilutti commented Jan 18, 2023

rem1776 commented Jan 19, 2023

laurenchilutti commented Jan 20, 2023

thomas-robinson left a comment

ganganoaa commented Jan 20, 2023 via email

thomas-robinson left a comment

Memory leak fix: pointers in interpolator and horiz_interp #1100

Memory leak fix: pointers in interpolator and horiz_interp #1100

Conversation

laurenchilutti commented Jan 3, 2023

laurenchilutti commented Jan 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganganoaa Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganganoaa commented Jan 3, 2023 via email

ganganoaa commented Jan 4, 2023 via email

thomas-robinson left a comment

Choose a reason for hiding this comment

ganganoaa commented Jan 11, 2023 via email

laurenchilutti commented Jan 18, 2023

rem1776 commented Jan 19, 2023

laurenchilutti commented Jan 20, 2023

thomas-robinson left a comment

Choose a reason for hiding this comment

ganganoaa commented Jan 20, 2023 via email

thomas-robinson left a comment

Choose a reason for hiding this comment

ganganoaa Jan 17, 2023 •

edited

Loading