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.

Sign up for GitHub

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

Jump to bottom

Release 25.01 #790

Merged
merged 4 commits into from
Jan 9, 2025
Merged

Release 25.01 #790

merged 4 commits into from
Jan 9, 2025

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Jan 9, 2025

Prepare the January release of ImpactX 🎉

@ax3l ax3l added the component: documentation Docs, readme and manual label Jan 9, 2025
@ax3l ax3l requested review from cemitch99 and EZoni January 9, 2025 06:29
@ax3l
Copy link
Member Author

ax3l commented Jan 9, 2025

AMReX Windows FFTW: will be addressed in the weekly update AMReX-Codes/amrex#4282
Compiles on CF & Spack also with the current 25.01 release of AMReX, so we can ignore it for this release.

@ax3l
Copy link
Member Author

ax3l commented Jan 9, 2025

@WeiqunZhang interesting, when we update AMReX to 25.01, our ASAN plugin shows a memory leak in amrex::FFT::OpenBCSolver

job-logs.zip

@ax3l ax3l mentioned this pull request Jan 9, 2025
Closed
cemitch99
cemitch99 reviewed Jan 9, 2025
View reviewed changes
cmake/dependencies/ABLASTR.cmake
CACHE STRING
"Repository branch for ImpactX_ablastr_repo if(ImpactX_ablastr_internal)")

# AMReX is transitively pulled through ABLASTR
set(ImpactX_amrex_repo "https://github.com/AMReX-Codes/amrex.git"
CACHE STRING
"Repository URI to pull and build AMReX from if(ImpactX_amrex_internal)")
set(ImpactX_amrex_branch "e64ffef57a7608d1d60f9abe738cc634e9c1272e"
set(ImpactX_amrex_branch ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify for my understanding: without a specification here, what version of AMReX will be used?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without specifying it here, we pick the version set in ABLASTR (WarpX):
https://github.com/ECP-WarpX/WarpX/blob/development/cmake/dependencies/AMReX.cmake#L293-L301

I usually only overwrite it here if we temporarily need a newer version than what is set there.

@ax3l ax3l mentioned this pull request Jan 9, 2025
Closed
@WeiqunZhang
Copy link
Member

https://www.fftw.org/faq/section3.html#leaks

Question 3.16. FFTW seems to have a memory leak.
After you create a plan, FFTW caches the information required to quickly recreate the plan. (See Q3.9 `Can I save FFTW's > plans?') It also maintains a small amount of other persistent memory. You can deallocate all of FFTW's internally allocated memory, if you wish, by calling fftw_cleanup(), as documented in the manual.

We could try to call fftw_cleanup.

@ax3l
Copy link
Member Author

ax3l commented Jan 9, 2025

Interesting, I never saw this issue in other FFTW runs. Sure, let's add this to AMReX finalization?

@WeiqunZhang
Copy link
Member

Could we try AMReX-Codes/amrex#4289 to see if it fixes the problem? If it does, I am actually not sure we should fix it.

@ax3l
Copy link
Member Author

ax3l commented Jan 9, 2025

I'll try.

I was wondering why linac-segment.run is the only test that shows the memory leak, and the reason might be that all other FFT tests in ImpactX are set to not use MPI.

@WeiqunZhang
Copy link
Member

The issue might be batched fftw plans. When there is no MPI, we use fftw to do the 3d fft. But with MPI, we fftw to do batched 1d ffts.

@WeiqunZhang
Copy link
Member

In that test, we call both fftw_plan_dft_?2? and fftwf_plan_many_dft_?2?. But the log shows it only complains about the latter.

@ax3l
Copy link
Member Author

ax3l commented Jan 9, 2025

Correct, same with valgrind.

@ax3l ax3l mentioned this pull request Jan 9, 2025
Closed
@WeiqunZhang
Copy link
Member

Okay, I found the issue. This is a real memory leak. Let me think about how to fix it.

@ax3l ax3l mentioned this pull request Jan 9, 2025
Closed
@WeiqunZhang WeiqunZhang mentioned this pull request Jan 9, 2025
Merged
@WeiqunZhang
Copy link
Member

AMReX-Codes/amrex#4290

@ax3l
CMake: AMReX_FFT Control
991fdce 
Through ABLASTR already, but just staying explicit here.
@ax3l
Copy link
Member Author

ax3l commented Jan 9, 2025

Thank you - awesome, that fixes the leaks we see here 🎉

ax3l pushed a commit to AMReX-Codes/amrex that referenced this pull request Jan 9, 2025
@WeiqunZhang
Fix a memory leak in Open BC solver (#4290)
041f225 
We forgot a test to make sure We only build the plans once.

X-Ref:
- ECP-WarpX/impactx#790
- ECP-WarpX/WarpX#5547
@ax3l ax3l mentioned this pull request Jan 9, 2025
Merged
1 task
@ax3l
Copy link
Member Author

ax3l commented Jan 9, 2025

Testing all new AMReX bugfixes in #791, but will merge this release PR first.

@ax3l ax3l merged commit a0e7154 into ECP-WarpX:development Jan 9, 2025
13 of 16 checks passed
@ax3l ax3l deleted the release-25.01 branch January 9, 2025 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cemitch99 cemitch99 cemitch99 left review comments

@EZoni EZoni Awaiting requested review from EZoni

Assignees
No one assigned
Labels
component: documentation Docs, readme and manual
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ax3l @WeiqunZhang @cemitch99