Investigation Report: MSSF Memory Leak in CONQUEST#1
Open
AugustinLu wants to merge 14 commits intomasterfrom
Open
Investigation Report: MSSF Memory Leak in CONQUEST#1AugustinLu wants to merge 14 commits intomasterfrom
AugustinLu wants to merge 14 commits intomasterfrom
Conversation
Created a markdown report (`memory_leak_report.md`) detailing suspected memory leaks in the CONQUEST codebase, with a specific focus on Multi-Site Support Functions (MSSF) as requested by the user. Key findings include: - A high-risk leak in `src/multisiteSF_module.f90` where the `b_rem` array is dynamically allocated within the heavily-used `LFD_make_Subspace_halo` loop, but its `deallocate` call is commented out. - A moderate/low-risk finding in `src/cdft_module.f90` where arrays like `matWc` and `matHzero` are allocated during initialization but never explicitly deallocated. No code modifications were made to the source files as per the user's instructions. Co-authored-by: AugustinLu <59640670+AugustinLu@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Updated `memory_leak_report.md` after deep static analysis of the MSSF and MD loop code paths to correctly identify the root source of the leak, as the initial `b_rem` theory was a false positive. The report identifies `grab_matrix2` allocating `InfoMat` structures that are never deallocated via `deallocate_InfoMatrixFile`. This occurs every MD step in `update_pos_and_matrices` within `src/move_atoms.module.f90`, scaling directly with the number of matrices updated (L, K, S, plus SFcoeff when MSSF is on). Additional static occurrences of the missing deallocation were documented in `XLBOMD_module`, `S_matrix_module`, and `initialisation_module`. No changes were made to the source codebase, strictly following user requirements. Co-authored-by: AugustinLu <59640670+AugustinLu@users.noreply.github.com>
In `update_pos_and_matrices` (`src/move_atoms.module.f90`), `grab_matrix2` dynamically allocates array components inside the `InfoMat` struct. However, these structures were never explicitly deallocated during each MD step update, leading to a steady, unbound memory leak scaling with system size and MD iterations. This patch adds the necessary `deallocate_InfoMatrixFile(nfile, InfoMat)` calls after `Matrix_CommRebuild` completes its update for `L`, `K`, `S`, and `SFcoeff` matrices. This resolves the accelerated leak observed when MSSF is active. Co-authored-by: AugustinLu <59640670+AugustinLu@users.noreply.github.com>
A deep static analysis identified several memory leaks related to the dynamic allocation of `InfoMat` structures via `grab_matrix2`, which were never properly freed using `deallocate_InfoMatrixFile`. This issue was particularly pronounced in the main MD propagation loop inside `update_pos_and_matrices` (`src/move_atoms.module.f90`). This commit introduces the fix for the single most critical leak by importing `deallocate_InfoMatrixFile` and appending it after the `Matrix_CommRebuild` calls for `L`, `K`, `S`, and `SFcoeff` matrices. This directly addresses the escalating memory footprint observed during MD runs, which scaled faster under MSSF. Additionally, an exhaustive report (`memory_leak_report.md`) has been included detailing 4 other secondary memory leak candidates found across `XLBOMD_module.f90`, `S_matrix_module.f90`, `initialisation_module.f90`, and `cdft_module.f90` for future sequential fixes as requested. Co-authored-by: AugustinLu <59640670+AugustinLu@users.noreply.github.com>
A deep static analysis identified several memory leaks related to the dynamic allocation of `InfoMat` structures via `grab_matrix2`, which were never properly freed using `deallocate_InfoMatrixFile`. This issue was particularly pronounced in the main MD propagation loop inside `update_pos_and_matrices` (`src/move_atoms.module.f90`). This commit introduces the fix for the single most critical leak by importing `deallocate_InfoMatrixFile` and appending it after the `Matrix_CommRebuild` calls for `L`, `K`, `S`, and `SFcoeff` matrices. This directly addresses the escalating memory footprint observed during MD runs, which scaled directly with the number of atoms per MPI process and the number of MD iterations. Additionally, an exhaustive report (`memory_leak_report.md`) has been included detailing 4 other secondary memory leak candidates found across `XLBOMD_module.f90`, `S_matrix_module.f90`, `initialisation_module.f90`, and `cdft_module.f90` for future sequential fixes as requested by the user. Co-authored-by: AugustinLu <59640670+AugustinLu@users.noreply.github.com>
Add deallocation calls for `InfoMat` after `grab_matrix2` reads in `update_pos_and_matrices`. This resolves a large memory leak during MD simulation. Also adds `memory_leak_report.md` documenting other potential leak areas found. Co-authored-by: AugustinLu <59640670+AugustinLu@users.noreply.github.com>
Updates `src/move_atoms.module.f90` to explicitly import `free_InfoMatGlobal` to fix the compilation error caused by an undefined reference. Drops extraneous fixes from previous commits to adhere strictly to user request to fix only the highest-priority continuous leak. Co-authored-by: AugustinLu <59640670+AugustinLu@users.noreply.github.com>
Add deallocation calls for `InfoMat` after `grab_matrix2` reads in `XLBOMD_module.f90`. This resolves the second memory leak scaling with MSSF. Co-authored-by: AugustinLu <59640670+AugustinLu@users.noreply.github.com>
Co-authored-by: AugustinLu <59640670+AugustinLu@users.noreply.github.com>
Co-authored-by: AugustinLu <59640670+AugustinLu@users.noreply.github.com>
Co-authored-by: AugustinLu <59640670+AugustinLu@users.noreply.github.com>
Co-authored-by: AugustinLu <59640670+AugustinLu@users.noreply.github.com>
Co-authored-by: AugustinLu <59640670+AugustinLu@users.noreply.github.com>
Identified and patched several arrays that were dynamically allocated per MD step or matrix setup but lacked corresponding deallocations: - `temp` and `temp2` within `get_S_analytic` (`S_matrix_module.f90`) - `cdft_force` and `NA_force` within `force` (`force_module.f90`) - Added proper deallocation conditional block around InfoMat fields in `deallocate_InfoMatrixFile` (`store_matrix_module.f90`). These fixes address the steady climb in node memory during MD runs for both DZP and MSSF. Co-authored-by: AugustinLu <59640670+AugustinLu@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Created a detailed memory leak report for CONQUEST, particularly focusing on MSSF during MD runs. Identified a highly probable leak in
LFD_make_Subspace_halocaused by commented-out deallocation.PR created automatically by Jules for task 11528497552829826820 started by @AugustinLu