Skip to content

Speedup MultiFab registry#6139

Closed
dpgrote wants to merge 2 commits intoBLAST-WarpX:developmentfrom
dpgrote:speedup_MultiFab_register
Closed

Speedup MultiFab registry#6139
dpgrote wants to merge 2 commits intoBLAST-WarpX:developmentfrom
dpgrote:speedup_MultiFab_register

Conversation

@dpgrote
Copy link
Copy Markdown
Member

@dpgrote dpgrote commented Sep 5, 2025

This PR is a possible solution to speed up the MultiFab registry, when getting MultiFabs.

Most MultiFabs in WarpX are specified by the enum FieldType, and when the MultiFab is fetched (using get), the enum is converted to a string since the MultiFab map keys are strings. This conversion is done using the amrex::getEnumNameString routine. That routine then internally calls amrex::getEnumNameValuePairs which generates a vector of the name, value pairs for whole enum type. Then getEnumNameString returns the one name requested. This means that every time a get is done, the whole name, value vector for the enum is generated.

The FieldType enum has 67 values, so creating that vector is somewhat expensive. For 2D and 3D, this time is not significant, but for 1D it can be. In a test case, this was taking up about half of the run time (since it was done almost 30 million times).

There are two relatively simple solutions. The first is to not use the FieldType enum, but always use strings for the MultiFabs. A second solution is to cache the names. This PR shows what that might look like.

This cache is not perfect. This assumes that the int values of each of the enums is unique. This is true in WarpX, but some other code could break this (by using more than one enum type for the MultiFabs for example).

@dpgrote dpgrote added performance optimization component: ABLASTR components shared with other PIC codes labels Sep 5, 2025
@ax3l ax3l changed the title [WIP]Speedup MultiFab registry [WIP] Speedup MultiFab registry Sep 5, 2025
@ax3l ax3l requested review from WeiqunZhang and ax3l September 5, 2025 18:34
@ax3l
Copy link
Copy Markdown
Member

ax3l commented Sep 5, 2025

Wow, good catch!!

Do we maybe want to implement a faster to-string routine in AMReX for AMReX_Enum, that only creates one string at a time, that we can use here?

@ax3l ax3l added the workaround label Sep 5, 2025
@RemiLehe
Copy link
Copy Markdown
Member

RemiLehe commented Sep 5, 2025

Interesting!
Is there a way to not call amrex::getEnumNameValuePairs for every call to amrex::getEnumNameString.
Could we call it only once at initialization?

@ax3l
Copy link
Copy Markdown
Member

ax3l commented Sep 5, 2025

This might achieve the same caching, but upstream :)
AMReX-Codes/amrex#4643

@AlexanderSinn
Copy link
Copy Markdown
Contributor

AlexanderSinn commented Sep 5, 2025

Just curious, could the key type of the m_mf_register map be changed from string to the enum type (int)? Or is the access by string still more common?

@dpgrote
Copy link
Copy Markdown
Member Author

dpgrote commented Sep 6, 2025

Just curious, could the key type of the m_mf_register map be changed from string to the enum type (int)? Or is the access by string still more common?

Access using the enum and string are needed. In some cases the same MF is accessed with the enum and string so it needs to be consistent. There are a number of MFs where the name is generated dynamically (for instance to include the species name).

@dpgrote dpgrote changed the title [WIP] Speedup MultiFab registry Speedup MultiFab registry Sep 9, 2025
WeiqunZhang pushed a commit to AMReX-Codes/amrex that referenced this pull request Sep 11, 2025
## Summary

The `getEnumNameValuePairs` logic used below a lot of `AMREX_ENUM`
functions is very slow for long-enough (e.g., N>70) enums.

In WarpX 1D simulations, it took up close to 50% of the runtime, as Dave
Grote found.

This caches the key-value string list on first creation, because the
type is known at compile-time.

A better implementation might skip the dictionary altogether for
functions that just need one string, like `getEnumNameString`.

## Additional background

BLAST-WarpX/warpx#6139
@JustinRayAngus
Copy link
Copy Markdown
Contributor

JustinRayAngus commented Sep 11, 2025

@ax3l While the recent commit to AMReX (AMReX-Codes/amrex#4643) improves things, it doesn't seem to be as big of an improvement compared to the changes in this PR.

Using the 1D uniform plasma test that I have showed during KISMET meetings, here are the wall times:
Original: 398 s
with AMReX/4643: 197 s
with this PR: 179 s

I've ran the tests several times and get the same result - This PR is about 10% faster.

@WeiqunZhang
Copy link
Copy Markdown
Member

Could you try this? AMReX-Codes/amrex#4654

@JustinRayAngus
Copy link
Copy Markdown
Contributor

Could you try this? AMReX-Codes/amrex#4654

That seems to work just as good as this PR.

@ax3l
Copy link
Copy Markdown
Member

ax3l commented Sep 11, 2025

Fixed upstream via AMReX-Codes/amrex#4654
Will be pulled in with the next weekly AMReX update.

@ax3l
Copy link
Copy Markdown
Member

ax3l commented Sep 11, 2025

Thank you @dpgrote @JustinRayAngus and @WeiqunZhang for identifying, testing and fixing this!

@ax3l ax3l closed this Sep 11, 2025
@ax3l
Copy link
Copy Markdown
Member

ax3l commented Sep 11, 2025

@JustinRayAngus @dpgrote if you have the 1D benchmark you used available and could add it to the WarpX CI tests, I would be curious to also benchmark it at some point :)

@dpgrote dpgrote deleted the speedup_MultiFab_register branch September 15, 2025 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants