Skip to content

Optimize some hot paths#603

Open
knoepfel wants to merge 6 commits into
Framework-R-D:mainfrom
knoepfel:minor-optimizations
Open

Optimize some hot paths#603
knoepfel wants to merge 6 commits into
Framework-R-D:mainfrom
knoepfel:minor-optimizations

Conversation

@knoepfel
Copy link
Copy Markdown
Member

These are some optimizations that were suggested by Claude using macOS Instruments' profile data on the many_events test.

  • Send the algorithm_name directly to product_store constructors instead of creating one from a std::string.
  • Adjust the underlying container of products to be a std::vector.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@            Coverage Diff             @@
##             main     #603      +/-   ##
==========================================
- Coverage   82.62%   82.40%   -0.23%     
==========================================
  Files         161      161              
  Lines        5895     5893       -2     
  Branches      682      681       -1     
==========================================
- Hits         4871     4856      -15     
- Misses        803      812       +9     
- Partials      221      225       +4     
Flag Coverage Δ
scripts 76.12% <ø> (ø)
unittests 85.64% <100.00%> (-0.35%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
phlex/core/consumer.cpp 100.00% <100.00%> (ø)
phlex/core/declared_fold.hpp 95.08% <100.00%> (-0.08%) ⬇️
phlex/core/declared_observer.hpp 100.00% <100.00%> (ø)
phlex/core/declared_predicate.hpp 94.73% <100.00%> (ø)
phlex/core/declared_transform.hpp 88.57% <100.00%> (ø)
phlex/core/declared_unfold.hpp 96.42% <100.00%> (ø)
phlex/core/edge_maker.cpp 77.27% <100.00%> (ø)
phlex/core/provider_node.cpp 100.00% <100.00%> (ø)
phlex/core/provider_node.hpp 100.00% <ø> (ø)
phlex/core/registrar.hpp 81.25% <100.00%> (ø)
... and 6 more

... and 4 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3455282...5676f0f. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@greenc-FNAL greenc-FNAL left a comment

Choose a reason for hiding this comment

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

Most of these comments reflect my unfamiliarity with the code rather than issues with the code itself.

Any chance of a test to mitigate https://app.codecov.io/gh/Framework-R-D/phlex/pull/603?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Framework-R-D?

Comment thread phlex/core/consumer.hpp Outdated
Comment thread phlex/core/declared_fold.hpp Outdated
Comment thread phlex/core/provider_node.cpp
Comment thread phlex/model/products.hpp Outdated
@knoepfel
Copy link
Copy Markdown
Member Author

Any chance of a test to mitigate https://app.codecov.io/gh/Framework-R-D/phlex/pull/603?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Framework-R-D?

The contains() interface was not used anywhere, so I've removed it. It will be straightforward to add it back if we need it.

@knoepfel knoepfel requested a review from greenc-FNAL May 19, 2026 15:49
@knoepfel knoepfel dismissed greenc-FNAL’s stale review May 19, 2026 15:53

Comments addressed in most recent commits

Comment thread phlex/core/declared_fold.hpp Outdated
@knoepfel knoepfel force-pushed the minor-optimizations branch from 6b9af5e to 93bb16f Compare May 20, 2026 17:15
@knoepfel knoepfel force-pushed the minor-optimizations branch from 93bb16f to 5676f0f Compare May 20, 2026 18:18
@knoepfel knoepfel requested a review from beojan May 21, 2026 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants