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

Increase in time consumption of the function GeoHandler::i_collect after PR #1277 #1291

Closed
1 task done
SanghyunKo opened this issue Jul 8, 2024 · 2 comments · Fixed by #1298
Closed
1 task done
Labels

Comments

@SanghyunKo
Copy link
Contributor

Check duplicate issues.

  • Checked for duplicates

Goal

To resolve a scalability issue caused by std::find for highly granular detectors

Operating System and Version

Alma 9

compiler

GCC 11

ROOT Version

6.30/06

DD4hep Version

nightly

Reproducer

I used my own detector for testing, but I bet you can reproduce it by scaling up the BoxOfStraws example without SD

Additional context

While moving from the old release to the nightly release, we realized that the overhead time of constructing geometry for a highly granular detector has significantly increased. After doing some tests, we tracked down PR #1277, which seems to be causing the issue. (the nightly build 2024-06-05 is fine, but the 2024-06-15 is too slow - and this is the only PR in DD4hep between the two)

We ran a callgrind profiler and figured out that GeoHandler::i_collect has significantly increased timing consumption. I have attached the results for two nightly builds below.
0605_result_callgrind.out.3261198.txt
0615_result_callgrind.out.1226121.txt

I assume that std::find here is having a scalability issue for high granular detectors because std::set::emplace has O(log(N)) but std::find has O(N) time complexity.

But, to be honest, I'm not sure how to fix this without ruining #1271 again... so I'd be happy to hear ideas from the developers.

FYI @BrieucF @atolosadelgado @swkim95 @s6anloes

@SanghyunKo SanghyunKo added the bug label Jul 8, 2024
@andresailer
Copy link
Member

I was afraid that would happen, and I checked the CLICdet / CLD detector and didn't notice anything getting worse, but of course that was only 80k or so volumes.
To fix that would be to store the order in the vector, and a second container for faster finding. As this stores only a pointer the memory overhead shouldn't be too bad.

@andresailer
Copy link
Member

Fixed by @SanghyunKo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants