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

Profiler: use amrex logic for device synchronization instead of custom solution #4889

Conversation

lucafedeli88
Copy link
Member

This PR is a follow-up of the discussion with @AlexanderSinn in #4880 .

Currently we implement in ablastr some logic to ensure device synchronization for the TinyProfiler.
However, after @AlexanderSinn 's PR AMReX-Codes/amrex#3763 , we can rely on what is already implemented in AMReX in order to achieve that.

Therefore, this PR does the following:

  • The AMReX logic is now used for device synchronization of the profiler. This is achieved by doing
 //https://github.com/AMReX-Codes/amrex/pull/3763
#ifdef AMREX_USE_GPU
        bool warpx_do_device_synchronize = true;
#else
        bool warpx_do_device_synchronize = false;
#endif
        pp_warpx.query("do_device_synchronize", warpx_do_device_synchronize);
        bool do_device_synchronize = warpx_do_device_synchronize;
        amrex::ParmParse pp_tiny_profiler("tiny_profiler");
        if (pp_tiny_profiler.queryAdd("device_synchronize_around_region", do_device_synchronize) )
        {
            WARPX_ALWAYS_ASSERT_WITH_MESSAGE(
                do_device_synchronize == warpx_do_device_synchronize,
                "tiny_profiler.device_synchronize_around_region overrides warpx.do_device_synchronize.");
        }

inside WarpXAMReXInit.cpp`.

  • WarpXProfilerWrapper.H and ablastr/profiler/ProfilerWrapper.H are now ultra-thin wrappers around the macros provided by AMReX.

  • WarpX::do_device_synchronize static variable is eliminated (with some minor changes to functions that required this variable as a parameter).

  • All the other changes are inclusions of dependencies that were previously included transitively via #include "WarpX.H" inside WarpXProfilerWrapper.H.

@lucafedeli88 lucafedeli88 added cleaning Clean code, improve readability component: ABLASTR components shared with other PIC codes labels Apr 24, 2024
@AlexanderSinn
Copy link
Member

Looks Great!

@ax3l ax3l requested a review from WeiqunZhang April 24, 2024 18:26
@ax3l
Copy link
Member

ax3l commented Apr 24, 2024

@lucafedeli88 thank you for this! Can you rebase pls? :)

@lucafedeli88
Copy link
Member Author

@lucafedeli88 thank you for this! Can you rebase pls? :)

that's done, @ax3l !

@ax3l ax3l self-assigned this Apr 29, 2024
@lucafedeli88
Copy link
Member Author

ping, @AlexanderSinn and @ax3l ;-) !

Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

LGTM. This new default might hide a couple of race conditions that we introduce in the future 😅

@lucafedeli88 @AlexanderSinn , did you have a chance to double check performance stays the same?

@ax3l ax3l merged commit 0e41d93 into ECP-WarpX:development May 29, 2024
45 checks passed
Haavaan pushed a commit to Haavaan/WarpX that referenced this pull request Jun 5, 2024
…m solution (ECP-WarpX#4889)

* Use amrex logic for device synchronization in tiny profiler instead of custom solution

* improve comment

* remove useless comment

* fix missing include

* fix bug

* fix bug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleaning Clean code, improve readability component: ABLASTR components shared with other PIC codes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants