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

Integrate involute surface into ORANGE construction #1342

Merged
merged 47 commits into from
Sep 5, 2024

Conversation

VHLM2001
Copy link
Contributor

Bring the implemented involute geometry from pull 210ce4422 into the runtime. Before pulling analysis of the different sections of the code should be looked at for compilation efficiency and checks to ensure that the involute code does not slow down other non-related sections when not called for.

@sethrj sethrj changed the title Integration of involute geometry as a runtime option Integrate involute surface into ORANGE runtime Jul 31, 2024
@sethrj sethrj self-assigned this Jul 31, 2024
@sethrj sethrj added enhancement New feature or request orange Work on ORANGE geometry engine labels Jul 31, 2024
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.

I think this is pretty close. Please check for consistency in ordering of all the surface and object variant helpers and tests.

test/orange/surf/SoftSurfaceEqual.test.cc Outdated Show resolved Hide resolved
test/orange/orangeinp/IntersectRegion.test.cc Outdated Show resolved Hide resolved
src/orange/surf/detail/SurfaceTranslator.cc Outdated Show resolved Hide resolved
src/orange/surf/detail/InvoluteSolver.hh Outdated Show resolved Hide resolved
src/orange/surf/SoftSurfaceEqual.cc Outdated Show resolved Hide resolved
src/orange/orangeinp/IntersectRegion.hh Outdated Show resolved Hide resolved
src/orange/orangeinp/IntersectRegion.hh Outdated Show resolved Hide resolved
src/orange/OrangeTypes.hh Show resolved Hide resolved
test/orange/orangeinp/IntersectRegion.test.cc Outdated Show resolved Hide resolved
test/orange/surf/SoftSurfaceEqual.test.cc Show resolved Hide resolved
@sethrj sethrj marked this pull request as ready for review August 13, 2024 18:06
src/orange/orangeinp/IntersectRegion.cc Outdated Show resolved Hide resolved
src/orange/orangeinp/IntersectRegion.cc Outdated Show resolved Hide resolved
src/orange/orangeinp/IntersectRegion.hh Outdated Show resolved Hide resolved
src/orange/orangeinp/IntersectRegion.hh Outdated Show resolved Hide resolved
src/orange/surf/Involute.hh Outdated Show resolved Hide resolved
src/orange/surf/SurfaceIO.cc Outdated Show resolved Hide resolved
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.

Forgot to post these before the end of your internship. I'll polish this PR off so we can merge it. Thanks and good luck!

src/orange/surf/Involute.hh Outdated Show resolved Hide resolved
src/orange/surf/detail/InvoluteSolver.hh Outdated Show resolved Hide resolved
test/orange/orangeinp/IntersectRegion.test.cc Outdated Show resolved Hide resolved
@sethrj
Copy link
Member

sethrj commented Aug 16, 2024

Well, the GPU performance (even when not using involutes) seems to take a substantial hit (~40% longer for testem3 without involutes!) from this branch. It's easy to see why from looking at the occupancy change in the initialize-tracks kernel:

 {
 "const_mem": 0,
 "heap_size": 8388608,
-"local_mem": 152,
-"max_blocks_per_cu": 5,
+"local_mem": 160,
+"max_blocks_per_cu": 2,
 "max_threads_per_block": 256,
-"max_warps_per_eu": 40,
+"max_warps_per_eu": 16,
 "name": "initialize-tracks",
-"num_regs": 48,
-"occupancy": 0.625,
+"num_regs": 92,
+"occupancy": 0.25,
 "print_buffer_size": 5242880,
 "stack_size": 1024,
 "threads_per_block": 256

and along-step-neutral:

 {
 "const_mem": 0,
 "heap_size": 8388608,
-"local_mem": 0,
+"local_mem": 288,
 "max_blocks_per_cu": 2,
 "max_threads_per_block": 256,
 "max_warps_per_eu": 16,
 "name": "along-step-neutral",
-"num_regs": 123,
+"num_regs": 128,
 "occupancy": 0.25,
 "print_buffer_size": 5242880,
 "stack_size": 1024,

The local memory usage goes way up and the occupancy goes way down (where it can). For now I'll try disabling the runtime code paths and see if performance goes back to normal.

@sethrj
Copy link
Member

sethrj commented Aug 19, 2024

@VHLM2001 Great work this summer! As we discussed last Friday, it's disappointing but unsurprising that this ends up being a burden on the GPU. Next week (?) I can see if we can disable the code paths on GPU so that we can get this merged (and let someone else work on optimization).

@sethrj sethrj force-pushed the involute-integration branch from 89a03fe to d8c172a Compare August 28, 2024 12:25
@sethrj sethrj changed the title Integrate involute surface into ORANGE runtime Integrate involute surface into ORANGE construction Aug 29, 2024
@sethrj
Copy link
Member

sethrj commented Aug 29, 2024

I just ran before and after this branch; it seems the runtime "not reachable" is doing its job, but the CPU still suffers a runtime hit; not sure why since it should hit a "throw" into a cold section of the code. There should be no difference...
rel-throughput

@sethrj sethrj force-pushed the involute-integration branch from 4c54170 to 46a7344 Compare September 5, 2024 12:07
@sethrj
Copy link
Member

sethrj commented Sep 5, 2024

OK with the latest update all is well at runtime!

rel-throughput

@sethrj sethrj enabled auto-merge (squash) September 5, 2024 15:52
@sethrj sethrj merged commit 8760ebe into celeritas-project:develop Sep 5, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request orange Work on ORANGE geometry engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants