Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: compile-time DetectorMeasurementInfo for SSS #2108

Merged
merged 10 commits into from
Aug 29, 2023

Conversation

LuisFelipeCoelho
Copy link
Member

This is my first attempt to implement compile-time DetectorMeasurementInfo for SSS, as alternative to PR #1985 and following the same approach by @CarloVarni #2013

@LuisFelipeCoelho LuisFelipeCoelho added the 🚧 WIP Work-in-progress label May 8, 2023
@LuisFelipeCoelho LuisFelipeCoelho changed the title refactor: compile-time DetectorMeasurementInfo for SSS refactor!: compile-time DetectorMeasurementInfo for SSS May 8, 2023
@LuisFelipeCoelho LuisFelipeCoelho added the Component - Core Affects the Core module label May 8, 2023
@LuisFelipeCoelho LuisFelipeCoelho added this to the v25.0.1 milestone May 8, 2023
@CarloVarni
Copy link
Collaborator

it looks good to me. Do you have an estimate of the timing gain due to this?

@LuisFelipeCoelho LuisFelipeCoelho modified the milestones: v25.0.1, next May 8, 2023
@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Merging #2108 (6063a55) into main (c535c5d) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2108      +/-   ##
==========================================
- Coverage   49.53%   49.52%   -0.01%     
==========================================
  Files         453      453              
  Lines       25692    25695       +3     
  Branches    11819    11813       -6     
==========================================
  Hits        12726    12726              
- Misses       4583     4586       +3     
  Partials     8383     8383              
Files Changed Coverage Δ
Core/include/Acts/Seeding/SeedFinder.hpp 0.00% <ø> (ø)
Core/include/Acts/Seeding/SeedFinder.ipp 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

Is this actually faster due to the template parameter? If this is consistent for the entire SP loop, the branch prediction should make a runtime if equally fast, I would think.

Also: is it possible to decouple the config changes from the other changes, somehow?

EDIT: Oh, I see this partially contains #2067.

@CarloVarni
Copy link
Collaborator

Indeed @paulgessinger the config changes are also in another PR. Not sure if we want them here or there. Concerning why we want to remove one of the config parameters see the discussion here: #2067 (comment)

Concerning the timing: we indeed expect the code to be faster with these compile-time if statements. This has already been demostrated with #2013 and by Luis' studies. He observed that having two separate functions for SSS and PPP makes us reache ~the same timing performances of Athena. The compile-time ifs are a way to avoid code duplications while achieving that

@paulgessinger
Copy link
Member

Thanks @CarloVarni.

I see, it's surprising to me, but ok. I really would think if the if variable doesn't change, it should effectively be skipped.
Anyway, the whole struct is templated already, so probably ok to template this function too.

@LuisFelipeCoelho
Copy link
Member Author

In the case of this PR I still need to test it the performance gain comes from the changes from #2067 or actually because of the if statements

@github-actions github-actions bot added the Stale label Jun 7, 2023
@CarloVarni CarloVarni added the Component - Examples Affects the Examples module label Jun 11, 2023
@paulgessinger paulgessinger modified the milestones: next, v28.0.0 Jul 5, 2023
@paulgessinger paulgessinger added the Breaking change This change breaks backwards compatibility label Jul 5, 2023
@LuisFelipeCoelho LuisFelipeCoelho marked this pull request as ready for review July 12, 2023 14:13
@LuisFelipeCoelho
Copy link
Member Author

from my comparison in Athena, just changing the if statements it improves around 2-3%

@paulgessinger
Copy link
Member

paulgessinger commented Jul 24, 2023

Could you do me a favor @LuisFelipeCoelho and recheck the timing where you keep everything as is in this PR but change the enum parameter to a runtime argument, rather than a template parameter?

Otherwise: should we get this in for v28?

@paulgessinger paulgessinger modified the milestones: v28.0.0, v29.0.0 Jul 28, 2023
@paulgessinger
Copy link
Member

paulgessinger commented Aug 25, 2023

What's the plan for this @LuisFelipeCoelho ?

paulgessinger
paulgessinger previously approved these changes Aug 28, 2023
@CarloVarni
Copy link
Collaborator

Hi @LuisFelipeCoelho there are merge conflicts. Can you resolve them?

@CarloVarni CarloVarni removed the 🚧 WIP Work-in-progress label Aug 28, 2023
@paulgessinger paulgessinger merged commit d72def3 into acts-project:main Aug 29, 2023
54 checks passed
@acts-project-service
Copy link
Collaborator

🔴 Athena integration test results

Build job with this PR failed!

Please investigate the build job for the pipeline!

@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change This change breaks backwards compatibility Breaks Athena build This PR breaks the Athena build Component - Core Affects the Core module Component - Examples Affects the Examples module Seeding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants