keep particle overallocation on restore state#897
keep particle overallocation on restore state#897PhilipDeegan merged 1 commit intoPHAREHUB:masterfrom
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new static method Changes
Sequence Diagram(s)sequenceDiagram
participant SolverPPC
participant ParticleArray
participant Map
SolverPPC->>Map: add_to(key, ParticleArray)
Map-->>SolverPPC: check if key exists
alt Key does not exist
Map->>Map: initialize entry with empty ParticleArray
end
Map->>Map: reserve capacity for new ParticleArray
Map->>Map: replace contents with ParticleArray
SolverPPC->>SolverPPC: saveState_ updates maps
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/amr/solvers/solver_ppc.hpp (2)
155-170: LGTM! Consider refining the overallocation strategy.The new
add_tomethod effectively preserves the overallocation of the sourceParticleArray, which can be beneficial for performance. However, as the comment suggests, the optimal amount of overallocation should be assessed.Consider implementing a more sophisticated overallocation strategy based on performance measurements. For example, you could use a factor of the original capacity:
v.reserve(static_cast<std::size_t>(ps.capacity() * 1.2)); // 20% overallocationAlternatively, you could implement a custom growth strategy based on the specific needs of your application.
225-227: LGTM! Consider a minor optimization for key generation.The use of the new
add_tomethod improves code maintainability and reduces the risk of errors. Good job on this refactoring!To potentially improve performance, consider pre-computing the base key string outside the loop:
std::string const baseKey = ss.str() + "_"; for (auto& pop : state.ions) { std::string const key = baseKey + pop.name(); add_to(tmpDomain, key, pop.domainParticles()); add_to(patchGhost, key, pop.patchGhostParticles()); }This avoids repeated string concatenation for each population.
src/core/numerics/ion_updater/ion_updater.hpp (1)
224-226: Preferconst auto&overauto&&in lambda parameter for clarityIn line 224, the lambda parameter in
inDomainBoxis declared asauto&& particleRange. Unless you need perfect forwarding or plan to accept both lvalues and rvalues, usingconst auto&enhances readability and ensures thatparticleRangeis not modified within the lambda.Apply the following change:
- auto inDomainBox = [&domainBox](auto&& particleRange) // + auto inDomainBox = [&domainBox](const auto& particleRange) //
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/amr/solvers/solver_ppc.hpp (2 hunks)
- src/core/numerics/ion_updater/ion_updater.hpp (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/amr/solvers/solver_ppc.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/numerics/ion_updater/ion_updater.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/amr/solvers/solver_ppc.hpp (2)
155-170: LGTM! Consider adding a comment about overallocation strategy.The
add_tomethod effectively manages particle addition while preserving overallocation. This approach can potentially improve performance by reducing the frequency of reallocations.Consider adding a comment explaining the rationale behind preserving overallocation and how the optimal amount of overallocation might be determined in the future. For example:
// Preserve overallocation to reduce reallocation frequency. // TODO: Assess and optimize the overallocation strategy for best performance.
Line range hint
155-227: Overall improvement in particle management and potential performance.The introduced changes, particularly the new
add_tomethod and its usage insaveState_, align well with the PR objectives of limiting reallocations in the ion updater. These modifications potentially reduce MPI waits during the process of filling ion ghosts, which should enhance the efficiency of the ion updater.The code maintains good C++ practices, improves expressiveness, and introduces a strategy for managing overallocation that can be further optimized in the future.
As the project evolves, consider the following:
- Implement benchmarks to measure the impact of the overallocation strategy on performance.
- Explore adaptive overallocation strategies that adjust based on runtime behavior.
- Document the overallocation approach in the project's performance optimization guide for future reference.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- src/amr/solvers/solver_ppc.hpp (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/amr/solvers/solver_ppc.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (1)
src/amr/solvers/solver_ppc.hpp (1)
225-227: Great refactoring! Improved code readability and potential performance.The use of the new
add_tomethod simplifies thesaveState_function and encapsulates the particle addition logic. This change not only improves code readability but also potentially enhances performance by leveraging the overallocation strategy.
|
fillIonghosts still under 2% since reverting to export particles
|
2 participants
keep initial capacity during save/restore state in solver ppc
Summary by CodeRabbit
New Features
Bug Fixes
Documentation