fix: Optimize EDM4hep particle reading #3978
Conversation
WalkthroughChanges made to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Examples/Io/EDM4hep/src/EDM4hepReader.cpp (1)
Line range hint
244-299: Memory handling for large particle collections, verify we must!For extremely large particle collections, consider monitoring memory usage you should. Though pre-allocation helps, temporary memory usage during sorting, doubled it is.
Consider these aspects:
- Monitor peak memory usage during sorting
- For very large collections, consider batch processing
- Memory pressure on systems with limited resources, watch for
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Examples/Io/EDM4hep/src/EDM4hepReader.cpp(3 hunks)
🔇 Additional comments (3)
Examples/Io/EDM4hep/src/EDM4hepReader.cpp (3)
244-247: Wise optimization of memory allocation, this is!
Pre-allocating memory with reserve based on collection size, prevent unnecessary reallocations it does. Performance benefits, this brings.
259-259: Efficient particle accumulation strategy, you have chosen!
Using push_back on pre-allocated vectors, better than individual insertions into flat_set it is. Copy operations, reduced they are.
Also applies to: 289-289
292-299: A single sort operation before container construction, powerful this approach is!
Batch sorting of accumulated particles, followed by single container construction, more efficient than repeated sorting during individual insertions it is. However, verify the impact on particle ordering we must.
✅ Verification successful
Safe and correct, this sorting approach is!
Examined the implementation thoroughly, I have. Clear it is that:
- Using
boost::container::flat_setforSimParticleContainer, ordered by particle ID it remains CompareParticleIdcomparator, consistent ordering byparticleId()it ensures- Preserves particle relationships it does, through
groupByPrimaryVertexandgroupBySecondaryVertexfunctions
The Force is strong with this implementation. Maintain proper particle ordering it does, while performance benefits from single sort operation it gains.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that particle ordering is preserved after sorting
# Look for any existing tests or assertions about particle ordering
# Search for tests related to particle ordering
rg -l "particle.*order" --type cpp --type hpp
# Search for CompareParticleId usage and implementation
ast-grep --pattern 'struct CompareParticleId {
$$$
}'
Length of output: 133
Script:
#!/bin/bash
# Search for CompareParticleId implementation with correct file extensions
rg -p "struct.*CompareParticleId" --type cpp --type-add "hpp:*.hpp"
# Search for particle ordering tests with correct file extensions
rg -p "particle.*order" --type cpp --type-add "hpp:*.hpp"
# Find and examine test files related to EDM4hepReader
fd "Test.*EDM4hep.*\.(cpp|hpp)$"
# Look for the implementation details of SimParticleContainer
rg -p "class.*SimParticleContainer" --type cpp --type-add "hpp:*.hpp"
Length of output: 3822
Script:
#!/bin/bash
# Examine the CompareParticleId implementation
cat Examples/Framework/include/ActsExamples/EventData/SimParticle.hpp
# Look for SimParticleContainer definition
rg -p "SimParticleContainer" Examples/Framework/include/ActsExamples/EventData/SimParticle.hpp
Length of output: 9619
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
Examples/Io/EDM4hep/src/EDM4hepReader.cpp (3)
260-260: Enhance error message with particle details, we must!More context in error messages, helpful it would be. Particle properties in logs, aid debugging they will.
- ACTS_ERROR("Particle " << inParticle.getObjectID().index - << " not found in particle map"); + ACTS_ERROR("Particle " << inParticle.getObjectID().index + << " not found in particle map. " + << "PDG: " << inParticle.getPDG() + << ", Status: " << inParticle.getGeneratorStatus());Also applies to: 290-290
293-299: Move semantics, utilize we should!Unnecessary copies, avoid we must. More efficient, move semantics are.
- SimParticleContainer particlesGenerator{particlesGeneratorUnordered.begin(), - particlesGeneratorUnordered.end()}; - SimParticleContainer particlesSimulated{particlesSimulatedUnordered.begin(), - particlesSimulatedUnordered.end()}; + SimParticleContainer particlesGenerator{ + std::make_move_iterator(particlesGeneratorUnordered.begin()), + std::make_move_iterator(particlesGeneratorUnordered.end())}; + SimParticleContainer particlesSimulated{ + std::make_move_iterator(particlesSimulatedUnordered.begin()), + std::make_move_iterator(particlesSimulatedUnordered.end())};
377-380: Track the time-based sorting enhancement, we must!Sorting by geometry ID implemented is, but time-based sorting mentioned in comments it is. Track this enhancement for future implementation, we should.
Would you like me to create a GitHub issue to track the implementation of time-based sorting for SimHits?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Examples/Io/EDM4hep/src/EDM4hepReader.cpp(5 hunks)
🔇 Additional comments (1)
Examples/Io/EDM4hep/src/EDM4hepReader.cpp (1)
245-248: Wise optimization, this is!
Prevent unnecessary reallocations, reserving capacity does. Memory fragmentation, it reduces. Performance benefits, it brings.
| std::ranges::sort(particlesGeneratorUnordered, detail ::CompareParticleId{}); | ||
| std::ranges::sort(particlesSimulatedUnordered, detail ::CompareParticleId{}); |
There was a problem hiding this comment.
| std::ranges::sort(particlesGeneratorUnordered, detail ::CompareParticleId{}); | |
| std::ranges::sort(particlesSimulatedUnordered, detail ::CompareParticleId{}); | |
| std::ranges::sort(particlesGeneratorUnordered, detail::CompareParticleId{}); | |
| std::ranges::sort(particlesSimulatedUnordered, detail::CompareParticleId{}); |
| } | ||
| } | ||
|
|
||
| std::ranges::sort(simHitsUnordered, detail ::CompareGeometryId{}); |
There was a problem hiding this comment.
| std::ranges::sort(simHitsUnordered, detail ::CompareGeometryId{}); | |
| std::ranges::sort(simHitsUnordered, detail::CompareGeometryId{}); |
|
We were previously adding the particles to the particle container (flat_set) one by one, which lead to a ton of copy operations. This now accumulates the particles in vectors, sorts them, and then produces the particle containers.
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
New Features
Bug Fixes