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

Compose caching functionality of LazySenseCalculator #1569

Merged
merged 22 commits into from
Jan 10, 2025

Conversation

esseivaju
Copy link
Contributor

@esseivaju esseivaju commented Jan 8, 2025

As discussed in #1565, have a simple LazySenseCalculator and a front-end with caching functionality. We could probably rework the original sense calculator as an EagerSenseCalculator.

@esseivaju esseivaju added enhancement New feature or request orange Work on ORANGE geometry engine labels Jan 8, 2025
Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

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

Nifty! (A test would be good.)

@esseivaju
Copy link
Contributor Author

I think we don't have any tests for SenseCalculator yet? It's indirectly tested through the SimpleUnitTracker. I can add that and will also update the documentation.

Copy link

github-actions bot commented Jan 8, 2025

Test summary

 4 215 files   6 517 suites   15m 1s ⏱️
 1 676 tests  1 670 ✅  6 💤 0 ❌
22 970 runs  22 890 ✅ 80 💤 0 ❌

Results for commit 5dacc44.

♻️ This comment has been updated with latest results.

@esseivaju esseivaju marked this pull request as ready for review January 9, 2025 02:48
@sethrj
Copy link
Member

sethrj commented Jan 9, 2025

@esseivaju https://github.com/celeritas-project/celeritas/blob/develop/test/orange/univ/detail/SenseCalculator.test.cc ? I guess I missed that you didn't add a "lazy" calculator test in the previous PR. Could you please add both?

@sethrj sethrj changed the title Compose caching functionnality of LazySenseCalculator Compose caching functionality of LazySenseCalculator Jan 9, 2025
@esseivaju esseivaju requested a review from sethrj January 9, 2025 22:54
@esseivaju
Copy link
Contributor Author

Okay I think we're good now

@esseivaju
Copy link
Contributor Author

Comparing the previous develop run (6ec7f84):

Problem Geometry GPU [1/s] CPU [1/s] GPU+G4 [1/s]
atlas-tilecal orange 1.12 (±0.00) 0.09 (±0.00) 1.04 (±0.01)
cms-hgcal orange 0.64 (±0.00) 0.06 (±0.00) 0.59 (±0.00)
testem15 orange 8.25 (±0.05) 0.43 (±0.00) 6.29 (±0.07)
testem15+field orange 7.80 (±0.04) 0.39 (±0.00) 5.85 (±0.04)
testem15+field+msc orange 6.55 (±0.02) 0.34 (±0.00) 5.18 (±0.06)
testem3-composite+field+msc orange 1.05 (±0.00) 0.11 (±0.00) 1.18 (±0.01)
testem3-composite+msc orange 2.14 (±0.00) 0.12 (±0.00) 1.88 (±0.00)
testem3-expanded+field+msc orange 1.05 (±0.01) 0.11 (±0.00) 1.18 (±0.01)
testem3-flat orange 2.98 (±0.00) 0.19 (±0.00) 2.72 (±0.02)
testem3-flat+field orange 2.15 (±0.01) 0.18 (±0.00) 2.13 (±0.00)
testem3-flat+field+msc orange 1.21 (±0.00) 0.12 (±0.00) 1.39 (±0.00)
testem3-flat+msc orange 2.20 (±0.00) 0.13 (±0.00) 1.98 (±0.00)

and switching to LazySenseCalculator except for complex_intersect which still uses SenseCalculator to store senses:

Problem Geometry GPU [1/s] CPU [1/s] GPU+G4 [1/s]
atlas-tilecal orange 1.22 (±0.00) 0.10 (±0.00) 1.11 (±0.00)
cms-hgcal orange 0.69 (±0.00) 0.07 (±0.00) 0.63 (±0.00)
testem15 orange 8.36 (±0.05) 0.43 (±0.00) 6.26 (±0.24)
testem15+field orange 7.88 (±0.04) 0.39 (±0.00) 5.96 (±0.10)
testem15+field+msc orange 6.56 (±0.02) 0.34 (±0.00) 5.13 (±0.08)
testem3-composite+field+msc orange 1.05 (±0.00) 0.11 (±0.00) 1.17 (±0.01)
testem3-composite+msc orange 2.15 (±0.00) 0.13 (±0.00) 1.86 (±0.00)
testem3-expanded+field+msc orange 1.05 (±0.01) 0.12 (±0.00) 1.18 (±0.00)
testem3-flat orange 3.30 (±0.01) 0.19 (±0.00) 2.96 (±0.01)
testem3-flat+field orange 2.31 (±0.01) 0.17 (±0.00) 2.20 (±0.01)
testem3-flat+field+msc orange 1.25 (±0.01) 0.12 (±0.00) 1.44 (±0.01)
testem3-flat+msc orange 2.37 (±0.00) 0.13 (±0.00) 2.09 (±0.00)

Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

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

Awesome! Great refactor, excellent parameterized test.

@sethrj
Copy link
Member

sethrj commented Jan 10, 2025

@esseivaju Did you happen to check whether the register/local usage changes?

@sethrj sethrj merged commit 108cd76 into celeritas-project:develop Jan 10, 2025
37 checks passed
@sethrj sethrj added minor Minor internal changes or fixes performance Changes for performance optimization and removed enhancement New feature or request labels Jan 10, 2025
@esseivaju
Copy link
Contributor Author

  • Initialize-tracks: local memory down from 184 to 176
  • along-step-general-linear-propagate-linear and along-step-neutral: registers up from 122 to 126
  • geo-boundary: local memory up from 128 to 152

@esseivaju esseivaju deleted the refactor-sensecalculators branch January 10, 2025 16:36
esseivaju added a commit to esseivaju/celeritas that referenced this pull request Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Minor internal changes or fixes orange Work on ORANGE geometry engine performance Changes for performance optimization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants