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 additional memory leaks in GEOS-Chem #2104

Merged
merged 1 commit into from
Jan 10, 2024
Merged

Conversation

yantosca
Copy link
Contributor

@yantosca yantosca commented Jan 5, 2024

Name and Institution (Required)

Name: Bob Yantosca
Institution: Harvard + GCST

Confirm you have reviewed the following documentation

Describe the update

This is a placeholder PR and should not yet be merged. I have made the base branch main but will change it later.

This is a companion PR to #2102. Configuring GEOS-Chem with -DSANITIZE=y has revealed the following memory leaks in GEOS-Chem and HEMCO:

=================================================================
==1495473==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 107464 byte(s) in 133 object(s) allocated from:
    #0 0x14815712993f in __interceptor_malloc ../../.././libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x1bd566d in __hco_filedata_mod_MOD_filedata_init /n/holyscratch01/jacob_lab/ryantosca/tests/cloudj/test_memory/CodeDir/src/HEMCO/src/Core/hco_filedata_mod.F90:174
    #2 0x202020202020201f  (<unknown module>)

Direct leak of 29248 byte(s) in 8 object(s) allocated from:
    #0 0x14815712993f in __interceptor_malloc ../../.././libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x10c438e in __histcontainer_mod_MOD_histcontainer_create /n/holyscratch01/jacob_lab/ryantosca/tests/cloudj/test_memory/CodeDir/src/GEOS-Chem/History/histcontainer_mod.F90:319
    #2 0x202020202020201f  (<unknown module>)

Direct leak of 3720 byte(s) in 1 object(s) allocated from:
    #0 0x14815712993f in __interceptor_malloc ../../.././libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x18f6ae1 in __state_chm_mod_MOD_init_state_chm /n/holyscratch01/jacob_lab/ryantosca/tests/cloudj/test_memory/CodeDir/src/GEOS-Chem/Headers/state_chm_mod.F90:803

Direct leak of 3160 byte(s) in 1 object(s) allocated from:
    #0 0x14815712993f in __interceptor_malloc ../../.././libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x18fe162 in __state_chm_mod_MOD_init_state_chm /n/holyscratch01/jacob_lab/ryantosca/tests/cloudj/test_memory/CodeDir/src/GEOS-Chem/Headers/state_chm_mod.F90:1027

SUMMARY: AddressSanitizer: 143592 byte(s) leaked in 143 allocation(s).

Leak 1 is in HEMCO and will be addressed by a separate PR at https://github.com/geoschem/hemco. We are still investigating.

Leak 2 is in the GEOS-Chem Classic History code. We are still investigating.

Leaks 3 and 4 were resolved by adding DEALLOCATE statements (see commit 78fc204).

Tagging @msulprizio @lizziel

Expected changes

This will be a zero-diff update that will remove memory leaks. It will not change results.

Reference(s)

N/A

Related Github Issue(s)

Headers/phot_container_mod.F90
- Deallocate the Phot object before setting it to NULL()

Headers/state_chm_mod.F90
- Deallocate State_Chm%AerMass before setting it to NULL()

CHANGELOG.md
- Updated accordingly

Signed-off-by: Bob Yantosca <[email protected]>
@yantosca yantosca added memory category: Bug Fix Fixes a previously-reported bug labels Jan 5, 2024
@yantosca yantosca self-assigned this Jan 5, 2024
@yantosca
Copy link
Contributor Author

yantosca commented Jan 9, 2024

@lizziel @msulprizio: I have traced the memory leak in the History component to routine MetaHistItem_Insert, in History/metahistitem_mod.F90. Specifically to this code:

!=======================================================================
! Initialize a METAHISTORY ITEM named "Head", which will become the
! head of the existing list. "Head" will contain a new HISTORY ITEM.
!=======================================================================
! Allocate the "Head" object
ALLOCATE( Head, STAT=RC )
IF ( RC /= GC_SUCCESS ) THEN
ErrMsg = 'Could not allocate "Head"!'
CALL GC_Error( ErrMsg, RC, ThisLoc )
RETURN
ENDIF
! Allocate the "Head%Item" field, which will hold the HISTORY ITEM
ALLOCATE( Head%Item, STAT=RC )
IF ( RC /= GC_SUCCESS ) THEN
ErrMsg = 'Could not allocate "Head%Item"!'
CALL GC_Error( ErrMsg, RC, ThisLoc )
RETURN
ENDIF
!=======================================================================
! Insert "Head" at the start of the existing linked list
!=======================================================================
! Save the HISTORY ITEM argument in the "Item" field of "Head"
Head%Item = Item
! The "Next" field of "Head" points to the current head of the list
Head%Next => Node
! Set "Head" as the new head of the linked list
Node => Head

The issue is that we allocate a local variable HEAD (to serve as the head of the linked list) but then we never deallocate it. So the memory is leaked. This might be a by-product of the original code on which I based the History linked lists (see: https://github.com/mapmeld/fortran-machine/blob/main/flibs-0.9/flibs/src/datastructures/linkedlist.f90, which has a similar issue).

It might take a bit of doing to be able to rethink solving this memory leak in the GCClassic History diagnostics. I propose to thus fix the memory leaks #3 and #4 and then return to solving #2 later.

@yantosca yantosca changed the title [WIP] Fix remaining memory leaks in GEOS-Chem [WIP] Fix additional memory leaks in GEOS-Chem Jan 9, 2024
@yantosca yantosca marked this pull request as ready for review January 9, 2024 15:01
@yantosca yantosca changed the base branch from main to dev/14.3.0 January 9, 2024 15:01
@yantosca yantosca changed the title [WIP] Fix additional memory leaks in GEOS-Chem Fix additional memory leaks in GEOS-Chem Jan 9, 2024
@yantosca
Copy link
Contributor Author

yantosca commented Jan 9, 2024

All GEOS-Chem Classic integration tests passed:

==============================================================================
GEOS-Chem Classic: Execution Test Results

GCClassic #c7aa8ee Merge pull request #48 for branch 'dev/no-diff-to-benchmark' into dev/14.3.0
GEOS-Chem #78fc20489 Fix memory leaks in State_Chm%AerMass and State_Chm%Phot
HEMCO     #0c7e10e Merge pull request #252 from branch 'origin/feature/geosit_meteorology_constants_file_year_change' into dev/3.8.0

Using 24 OpenMP threads
Number of execution tests: 26

Submitted as SLURM job: 14850037
==============================================================================

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
%%%  All execution tests passed!  %%%
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

@msulprizio msulprizio merged commit ceda1a5 into dev/14.3.0 Jan 10, 2024
@msulprizio msulprizio deleted the bugfix/memory-leaks branch January 10, 2024 12:49
@msulprizio msulprizio added this to the 14.3.0 milestone Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: Bug Fix Fixes a previously-reported bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leaks in 14.3.0 development code
2 participants