Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Aug 22, 2025

Briefly, what does this PR introduce?

This PR addresses issue #1951 by refactoring the ParticleSvc to significantly improve compilation performance while moving the service to the appropriate location in the codebase. The large kParticleMap (250+ particle entries) has been moved from the header file to a separate implementation file, eliminating the need for GCC compilation pragmas and reducing header bloat.

Problem Statement

The current ParticleSvc.h contains a large inline static const ParticleMap kParticleMap with 250+ particle definitions that gets included in every downstream compilation unit. This significantly slows compilation, requiring GCC pragmas as a workaround:

#pragma GCC optimize("no-var-tracking") // to speed up compilation
inline static const ParticleMap kParticleMap{
    // 250+ particle entries here
};

Solution

Architectural Changes:

  • Moved ParticleSvc from src/algorithms/interfaces/ to src/services/particle/
  • Separated interface from implementation using factory pattern
  • Created lightweight header with implementation in separate translation unit

Before (297 lines):

// ParticleSvc.h - everything in header
class ParticleSvc {
  inline static const ParticleMap kParticleMap{
    // 250+ particles defined here - included in every compilation unit
  };
};

After (40 lines header + 268 lines implementation):

// ParticleSvc.h - lightweight interface only
class ParticleSvc {
  virtual void init(std::shared_ptr<ParticleMap> map = nullptr);
  // ... other interface methods
private:
  static const ParticleMap& getDefaultParticleMap(); // Implemented in .cc
};

// ParticleSvc.cc - implementation with particle data
const ParticleSvc::ParticleMap& ParticleSvc::getDefaultParticleMap() {
  static const ParticleMap kParticleMap{
    // All 250+ particles here - compiled once
  };
  return kParticleMap;
}

Performance Impact

  • Header size reduction: 297 → 40 lines (86% reduction)
  • Compilation: 250+ particle entries no longer parsed in every downstream file
  • Build time: Should significantly improve compilation performance
  • Memory: Reduced preprocessor memory usage during compilation

API Compatibility

Zero breaking changes - maintains identical public interface
Backward compatibility - old include path forwards to new location
Same functionality - all existing behavior preserved

Files Changed

New Structure:

src/services/particle/
├── CMakeLists.txt           # Build configuration
├── ParticleSvc.h            # Lightweight interface (40 lines)
└── ParticleSvc.cc           # Implementation with particle map (268 lines)

Updated Dependencies:

  • 6+ CMakeLists.txt files updated with particle_service_library dependency
  • 18+ source files updated to use new include path
  • Build system properly configured for new service library

Migration Path

Immediate: No action required - backward compatibility maintained via forwarding header

Recommended: Update includes from:

#include <algorithms/interfaces/ParticleSvc.h>  // old
#include <services/particle/ParticleSvc.h>      // new

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

  • Maintains API compatibility
  • Build configuration updated
  • No breaking changes for downstream users
  • Addresses compilation performance issues

Does this PR introduce breaking changes?

No. Full backward compatibility is maintained through a forwarding header at the original location.

Does this PR change default behavior?

No. All existing functionality and behavior is preserved exactly as before.

Result: This refactoring successfully eliminates the large particle map from downstream compilation units while moving ParticleSvc to the services library as requested, providing significant compilation performance improvements without any breaking changes.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@Copilot Copilot AI changed the title [WIP] Come up with a strategy for @eic/EICrecon/issues/1951 that does not revert the referenced commit, but that refactors the code in such a way that the inline static const ParticleMap kParticleMap inside src/algorithms/interfaces/ParticleSvc.h is not in... refactor: Move ParticleSvc to services library and separate interface from implementation Aug 22, 2025
Copilot finished work on behalf of wdconinc August 22, 2025 14:19
@Copilot Copilot AI requested a review from wdconinc August 22, 2025 14:19
@github-actions github-actions bot added topic: tracking Relates to tracking reconstruction topic: PID Relates to PID reconstruction topic: far-backward Reconstruction related to far backward detectors topic: infrastructure labels Aug 22, 2025
@wdconinc wdconinc added the pre-commit.ci autofix Enable pre-commit.ci autofixes even for bot accounts label Aug 22, 2025
@pre-commit-ci pre-commit-ci bot removed the pre-commit.ci autofix Enable pre-commit.ci autofixes even for bot accounts label Aug 22, 2025
@Copilot Copilot AI requested a review from wdconinc August 22, 2025 14:32
Copilot finished work on behalf of wdconinc August 22, 2025 14:32
@wdconinc wdconinc added the pre-commit.ci autofix Enable pre-commit.ci autofixes even for bot accounts label Aug 22, 2025
@pre-commit-ci pre-commit-ci bot removed the pre-commit.ci autofix Enable pre-commit.ci autofixes even for bot accounts label Aug 22, 2025
Copy link
Contributor

@wdconinc wdconinc left a comment

Choose a reason for hiding this comment

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

@copilot Please sort the headers such that the new services/particle/ParticleSvc.h header comes alphabetically after the other listed headers in algorithms/.

wdconinc and others added 2 commits August 22, 2025 09:49
… from implementation (fix: iwyu) (#2033)

This PR applies the include-what-you-use fixes as suggested by
https://github.com/eic/EICrecon/actions/runs/17157891534.
Please merge this PR into the branch
`copilot/fix-24a45d04-d181-469d-854f-d54ba58ad148`
to resolve failures in PR #2032.

Auto-generated by [create-pull-request][1]

[1]: https://github.com/peter-evans/create-pull-request

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Aug 22, 2025

@copilot Please sort the headers such that the new services/particle/ParticleSvc.h header comes alphabetically after the other listed headers in algorithms/.

Sorted headers alphabetically so that services/particle/ParticleSvc.h now comes after algorithms/ headers in all include blocks. Updated 9 files with proper alphabetical ordering. Commit: a57ef76

@Copilot Copilot AI requested a review from wdconinc August 22, 2025 14:54
Copilot finished work on behalf of wdconinc August 22, 2025 14:54
@Copilot Copilot AI temporarily deployed to github-pages August 23, 2025 01:19 Inactive
@wdconinc wdconinc marked this pull request as ready for review August 23, 2025 01:31
@wdconinc wdconinc requested a review from veprbl August 23, 2025 01:32
@wdconinc wdconinc enabled auto-merge August 23, 2025 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: far-backward Reconstruction related to far backward detectors topic: infrastructure topic: PID Relates to PID reconstruction topic: tracking Relates to tracking reconstruction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants