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

[WIP] Use singleton to store warpx profiler parameter "do_device_synchronize" #4880

Conversation

lucafedeli88
Copy link
Member

@lucafedeli88 lucafedeli88 commented Apr 22, 2024

Right now Source/Utils/WarpXProfilerWrapper.H includes the huge header WarpX.H, just because we need to pass WarpX::do_device_synchronize to ABLASTR_PROFILE.

This PR avoids this issue by introducing a new singleton class warpx::profiler::ProfileSettings (singletons make sense for loggers and profilers in my opinion) that stores this parameter (it might store additional parameters in the future). So, during the initialization, the WarpX class simply calls the function warpx::profiler::ProfileSettings::InitProfileSettings(do_device_synchronize); , and the parameter do_device_synchronize is stored inside ProfileSettings. This allows us to avoid the transitive inclusion of WarpX.H every time the profiler is used.
In some cases, including WarpX.H (or other headers included by WarpX.H) is now required because Utils/WarpXProfilerWrapper.H" no longer includes such libraries.

@lucafedeli88 lucafedeli88 added the cleaning Clean code, improve readability label Apr 22, 2024
@AlexanderSinn
Copy link
Member

A while ago, I fixed the synchronizes in amrex::TinyProfiler itself to be ordered correctly with respect to collecting the time AMReX-Codes/amrex#3763 . This means that instead of using the custom added syncs in ABLASTR the ones build in to amrex::TinyProfiler can be used now (Note that a small difference with this is that profiles in amrex would also get syncronizes). So most of the profiler wrapper stuff in WarpX/ABLASTR can be replaced with a small function that passes the input parameter from  warpx.do_device_synchronize = 1 to tiny_profiler.device_synchronize_around_region = 1.

@lucafedeli88
Copy link
Member Author

lucafedeli88 commented Apr 22, 2024

A while ago, I fixed the synchronizes in amrex::TinyProfiler itself to be ordered correctly with respect to collecting the time AMReX-Codes/amrex#3763 . This means that instead of using the custom added syncs in ABLASTR the ones build in to amrex::TinyProfiler can be used now (Note that a small difference with this is that profiles in amrex would also get syncronizes). So most of the profiler wrapper stuff in WarpX/ABLASTR can be replaced with a small function that passes the input parameter from warpx.do_device_synchronize = 1 to tiny_profiler.device_synchronize_around_region = 1.

Thanks for the information, @AlexanderSinn ! I was not aware of that, sorry... I think that if we can safely remove a wrapper layer we should do that. @ax3l , is the profiler in ABLASTR used also in ImpactX? If it is the case we may need to coordinate on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleaning Clean code, improve readability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants