Fixing YAXT index list related memory leaks in generation of redistribution map

[DrTVockerodtMO]

PR Summary

Sci/Tech Reviewer: Lorenzo Milazzo (@mo-lormi)
Code Reviewer: Andrew Coughtrie (@andrewcoughtrie)

The locally scoped source and target index lists used to generate the redistribution map are allocated but never freed. YAXT has routines to free the memory of its index lists which are used in the same file in the generation of the exchange maps. I have replicated this for the redistribution maps.

EDIT: These index lists actually do not appear to be doing anything in this routine and have been removed instead.

• fixes YAXT index list related memory leaks in generation of redistribution maps #449

Code Quality Checklist

• I have performed a self-review of my own code
• My code follows the project's style guidelines
• Comments have been included that aid understanding and enhance the readability of the code
• My changes generate no new warnings
• All automated checks in the CI pipeline have completed successfully

Testing

• I have tested this change locally, using the LFRic Core rose-stem suite
• If required (e.g. API changes) I have also run the LFRic Apps test suite using this branch
• If any tests fail (rose-stem or CI) the reason is understood and acceptable (e.g. kgo changes)
• I have added tests to cover new functionality as appropriate (e.g. system tests, unit tests, etc.)
• Any new tests have been assigned an appropriate amount of compute resource and have been allocated to an appropriate testing group (i.e. the developer tests are for jobs which use a small amount of compute resource and complete in a matter of minutes)

trac.log

Test Suite Results - lfric_core - mem_leak_idxlists/run2

Suite Information

Item	Value
Suite Name	mem_leak_idxlists/run2
Suite User	terence.vockerodt
Workflow Start	2026-08-18T09:37:19
Groups Run	developer

Dependency	Reference	Main Like
lfric_core	DrTVockerodtMO/lfric_core@mem_leak_idxlists	False
SimSys_Scripts	MetOffice/SimSys_Scripts@cab3315	True

Task Information

✅ succeeded tasks - 433

Test Suite Results - lfric_apps - linked_mem_leak_idxlists/run2

Suite Information

Item	Value
Suite Name	linked_mem_leak_idxlists/run2
Suite User	terence.vockerodt
Workflow Start	2026-08-18T10:49:26
Groups Run	developer

Dependency	Reference	Main Like
casim	MetOffice/casim@2026.07.1	True
jules	MetOffice/jules@2026.07.1	True
lfric_apps	DrTVockerodtMO/lfric_apps@linked_mem_leak_idxlists	False
lfric_core	DrTVockerodtMO/lfric_core@mem_leak_idxlists	True
moci	MetOffice/moci@2026.07.1	True
SimSys_Scripts	MetOffice/SimSys_Scripts@77a5166	True
socrates	MetOffice/socrates@2026.07.1	True
socrates-spectral	MetOffice/socrates-spectral@2026.07.1	True
ukca	MetOffice/ukca@9fc2b6d	True

Task Information

✅ succeeded tasks - 1218

Security Considerations

• I have reviewed my changes for potential security issues
• Sensitive data is properly handled (if applicable)
• Authentication and authorisation are properly implemented (if applicable)

Performance Impact

• Performance of the code has been considered and, if applicable, suitable performance measurements have been conducted

AI Assistance and Attribution

• Some of the content of this change has been produced with the assistance of Generative AI tool name (e.g., Met Office Github Copilot Enterprise, Github Copilot Personal, ChatGPT GPT-4, etc) and I have followed the Simulation Systems AI policy (including attribution labels)

Documentation

• Where appropriate I have updated documentation related to this change and confirmed that it builds correctly

PSyclone Approval

• If you have edited any PSyclone-related code (e.g. PSyKAl-lite, Kernel interface, optimisation scripts, LFRic data structure code) then please contact the TCD Team

Sci/Tech Review

• I understand this area of code and the changes being added
• The proposed changes correspond to the pull request description
• Documentation is sufficient (do documentation papers need updating)
• Sufficient testing has been completed

(Please alert the code reviewer via a tag when you have approved the SR)

Code Review

• All dependencies have been resolved
• Related Issues have been properly linked and addressed
• CLA compliance has been confirmed
• Code quality standards have been met
• Tests are adequate and have passed
• Documentation is complete and accurate
• Security considerations have been addressed
• Performance impact is acceptable

[Lorenzo Milazzo (mo-lormi)]

DrTVockerodtMO I imagine that a test using a memory leak detection tool has been carried out after applying this bug fix. It would be great if you could add a link to the log file associated to the test or if you could just report here the key info (summary) from this file.
Thanks ...

[DrTVockerodtMO]

DrTVockerodtMO I imagine that a test using a memory leak detection tool has been carried out after applying this bug fix. It would be great if you could add a link to the log file associated to the test or if you could just report here the key info (summary) from this file. Thanks ...

Good point. I ran a simplified version of the adjoint_tests application using Valgrind and found the following results:

Before

==142285== LEAK SUMMARY:
==142285==    definitely lost: 22,703,648 bytes in 509 blocks
==142285==    indirectly lost: 43,379 bytes in 398 blocks
==142285==      possibly lost: 1,467,592 bytes in 4 blocks
==142285==    still reachable: 64,529,617 bytes in 2,433 blocks
==142285==         suppressed: 0 bytes in 0 blocks
==142285== 

After

==212503== LEAK SUMMARY:
==212503==    definitely lost: 101,152 bytes in 457 blocks
==212503==    indirectly lost: 43,683 bytes in 398 blocks
==212503==      possibly lost: 1,056 bytes in 3 blocks
==212503==    still reachable: 64,529,617 bytes in 2,433 blocks
==212503==         suppressed: 0 bytes in 0 blocks

In the first scenario, there were blocks present that lead me to the memory leak that looked like:

==1545074== 581,040 bytes in 6 blocks are definitely lost in loss record 634 of 670
==1545074==    at 0x484882F: malloc (vg_replace_malloc.c:446)
==1545074==    by 0x48DC240: Xt_xmalloc (xmalloc.c:69)
==1545074==    by 0x48C9902: idxvec_alloc_no_init (xt_idxvec.c:189)
==1545074==    by 0x48C9902: idxvec_alloc (xt_idxvec.c:208)
==1545074==    by 0x48C9986: xt_idxvec_new (xt_idxvec.c:226)
==1545074==    by 0x4873828: __xt_idxvec_MOD_xt_idxvec_new_a1d_i4 (xt_idxvec_f.f90:145)
==1545074==    by 0x7231D2: __halo_comms_mod_MOD_generate_redistribution_map (halo_comms_mod.F90:848)
==1545074==    by 0x725944: __halo_comms_mod_MOD_halo_routing_constructor (halo_comms_mod.F90:343)
==1545074==    by 0x566560: __halo_routing_collection_mod_MOD_get_halo_routing (halo_routing_collection_mod.f90:200)
==1545074==    by 0x558C22: __field_parent_mod_MOD_get_halo_routing (field_parent_mod.f90:431)
==1545074==    by 0x553B0B: __field_int32_mod_MOD_halo_exchange (field_int32_mod.f90:761)
==1545074==    by 0x1895AF3: __psykal_lite_gen_lookup_tables_psy_mod_MOD_invoke_gen_a_h_o_lookup_kernel (psykal_lite_gen_adj_lookup_tables_mod.f90:806)
==1545074==    by 0x13B05C0: __adj_lookup_table_generators_alg_mod_MOD_create_lookup_apply_helmholtz_op (adj_lookup_table_generators_alg_mod.f90:384)

After making the change, I cannot find any reference to xt_idxvec_new in the log file. It also doesn't appear to have introduced any new leaks with the YAXT routines as the remaining routines it complains about are present in both files and in the same quantities.

[Lorenzo Milazzo (mo-lormi)]

I have looked at the codebase and the changes that you are proposing.
I might be wrong but I don't think that these changes will fix the memory leakage problem.
Calling the (Yaxt) destructor here and here is likely to result in undesirable side effects.
(I suggest to read carefully the comments in the FCM ticket 4476).

I believe that the current memory leak originates from another part of the codebase, possibly in a different file. Therefore, addressing this issue requires further investigation.

[DrTVockerodtMO]

I have looked at the codebase and the changes that you are proposing. I might be wrong but I don't think that these changes will fix the memory leakage problem. Calling the (Yaxt) destructor here and here is likely to result in undesirable side effects. (I suggest to read carefully the comments in the FCM ticket 4476).

I believe that the current memory leak originates from another part of the codebase, possibly in a different file. Therefore, addressing this issue requires further investigation.

Thanks for having a look. A couple of questions:

It's my general understanding that for objects the clean-up of memory ought to happen in the destructor. But the src_idxlist and tgt_idxlist are locally scoped and it was my impression that they need to be cleaned up via the YAXT destructor before they went out of scope else they would be inaccessible. Is that still true?

I think the only case where we wouldn't need to do this is if the index lists freed the memory in their own destructor, which ought to be called as they are going out of scope. In which case, https://github.com/DrTVockerodtMO/lfric_core/blob/4de4f3d9b7b430181f1f93534f21eb256b19c138/infrastructure/source/utilities/halo_comms_mod.F90#L910 and https://github.com/DrTVockerodtMO/lfric_core/blob/4de4f3d9b7b430181f1f93534f21eb256b19c138/infrastructure/source/utilities/halo_comms_mod.F90#L911 should also need to be removed no? These were not added by this PR and have been there for quite some time, even back to the ticket you linked.

Which file did you have in mind for where the memory leak would originate?

[DrTVockerodtMO]

On a second look it actually doesn't seem that the src_idxlist and tgt_idxlist are even doing anything for this particular routine so they should probably be removed.

[DrTVockerodtMO]

On a second look it actually doesn't seem that the src_idxlist and tgt_idxlist are even doing anything for this particular routine so they should probably be removed.

I can confirm that this passes the test-suite which is now attached, and also produces the same output as the previous solution did when using Valgrind.

[Andrew Coughtrie (andrewcoughtrie)]

On a second look it actually doesn't seem that the src_idxlist and tgt_idxlist are even doing anything for this particular routine so they should probably be removed.

I can confirm that this passes the test-suite which is now attached, and also produces the same output as the previous solution did when using Valgrind.

You should also run the lfric_apps test-suite against this change as well, historically it has been found that changes to halo comms can cause problems with lfric apps without triggering failures in core. Hopefully those holes in the testing have been filled now but it's best to be careful in this case.

[DrTVockerodtMO]

On a second look it actually doesn't seem that the src_idxlist and tgt_idxlist are even doing anything for this particular routine so they should probably be removed.

I can confirm that this passes the test-suite which is now attached, and also produces the same output as the previous solution did when using Valgrind.

You should also run the lfric_apps test-suite against this change as well, historically it has been found that changes to halo comms can cause problems with lfric apps without triggering failures in core. Hopefully those holes in the testing have been filled now but it's best to be careful in this case.

I did this on the lfric_apps developer suite for the first commit and everything passed, I will also run it with the newest changes.

[DrTVockerodtMO]

On a second look it actually doesn't seem that the src_idxlist and tgt_idxlist are even doing anything for this particular routine so they should probably be removed.

I can confirm that this passes the test-suite which is now attached, and also produces the same output as the previous solution did when using Valgrind.

You should also run the lfric_apps test-suite against this change as well, historically it has been found that changes to halo comms can cause problems with lfric apps without triggering failures in core. Hopefully those holes in the testing have been filled now but it's best to be careful in this case.

I did this on the lfric_apps developer suite for the first commit and everything passed, I will also run it with the newest changes.

Added this to the PR description!

[Lorenzo Milazzo (mo-lormi)]

Thanks for the further info ...

Have you used a memory leak detection tool after applying these latest changes?
If yes, could you share the check results? Does the memory leak still persist?

[DrTVockerodtMO]

Thanks for the further info ...

Have you used a memory leak detection tool after applying these latest changes? If yes, could you share the check results? Does the memory leak still persist?

I can share the results sure. Again, I ran a simplified version of the adjoint_tests application using Valgrind and found the following results:

Before this PR

==142285== LEAK SUMMARY:
==142285==    definitely lost: 22,703,648 bytes in 509 blocks
==142285==    indirectly lost: 43,379 bytes in 398 blocks
==142285==      possibly lost: 1,467,592 bytes in 4 blocks
==142285==    still reachable: 64,529,617 bytes in 2,433 blocks
==142285==         suppressed: 0 bytes in 0 blocks
==142285== 

After first set of changes (deallocating index lists)

==212503== LEAK SUMMARY:
==212503==    definitely lost: 101,152 bytes in 457 blocks
==212503==    indirectly lost: 43,683 bytes in 398 blocks
==212503==      possibly lost: 1,056 bytes in 3 blocks
==212503==    still reachable: 64,529,617 bytes in 2,433 blocks
==212503==         suppressed: 0 bytes in 0 blocks

After second set of changes (removing unused index lists)

==194378== LEAK SUMMARY:
==194378==    definitely lost: 102,416 bytes in 457 blocks
==194378==    indirectly lost: 42,419 bytes in 398 blocks
==194378==      possibly lost: 1,056 bytes in 3 blocks
==194378==    still reachable: 64,529,617 bytes in 2,433 blocks
==194378==         suppressed: 0 bytes in 0 blocks

The first and second sets of changes are practically identical and indicate a definite reduction in the memory leaked. In the first scenario (before this PR), there were blocks present that lead me to the memory leak that looked like:

==1545074== 581,040 bytes in 6 blocks are definitely lost in loss record 634 of 670
==1545074==    at 0x484882F: malloc (vg_replace_malloc.c:446)
==1545074==    by 0x48DC240: Xt_xmalloc (xmalloc.c:69)
==1545074==    by 0x48C9902: idxvec_alloc_no_init (xt_idxvec.c:189)
==1545074==    by 0x48C9902: idxvec_alloc (xt_idxvec.c:208)
==1545074==    by 0x48C9986: xt_idxvec_new (xt_idxvec.c:226)
==1545074==    by 0x4873828: __xt_idxvec_MOD_xt_idxvec_new_a1d_i4 (xt_idxvec_f.f90:145)
==1545074==    by 0x7231D2: __halo_comms_mod_MOD_generate_redistribution_map (halo_comms_mod.F90:848)
==1545074==    by 0x725944: __halo_comms_mod_MOD_halo_routing_constructor (halo_comms_mod.F90:343)
==1545074==    by 0x566560: __halo_routing_collection_mod_MOD_get_halo_routing (halo_routing_collection_mod.f90:200)
==1545074==    by 0x558C22: __field_parent_mod_MOD_get_halo_routing (field_parent_mod.f90:431)
==1545074==    by 0x553B0B: __field_int32_mod_MOD_halo_exchange (field_int32_mod.f90:761)
==1545074==    by 0x1895AF3: __psykal_lite_gen_lookup_tables_psy_mod_MOD_invoke_gen_a_h_o_lookup_kernel (psykal_lite_gen_adj_lookup_tables_mod.f90:806)
==1545074==    by 0x13B05C0: __adj_lookup_table_generators_alg_mod_MOD_create_lookup_apply_helmholtz_op (adj_lookup_table_generators_alg_mod.f90:384)

These blocks are once again no longer present in either Valgrind logs of both changesets, indicating this particular leak relating to L848 of halo_comms_mod.F90 was solved.

[Lorenzo Milazzo (mo-lormi)]

Thanks for sharing the output of the memory checks.

If I understand correctly, these changes address some (but not all) of the memory leaks.
Moreover, all tests for both lfric_core and lfric_apps have passed successfully.
If this is the case, I approve these changes.

[DrTVockerodtMO]

Thanks for sharing the output of the memory checks.

If I understand correctly, these changes address some (but not all) of the memory leaks. Moreover, all tests for both lfric_core and lfric_apps have passed successfully. If this is the case, I approve these changes.

Thank you. It is correct that this only addresses part of the memory leaks of that file, I wanted to keep the PR agile. :)