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

Draft: Pack bodies in the System #44

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Draft: Pack bodies in the System #44

wants to merge 5 commits into from

Conversation

gonzalobg
Copy link
Collaborator

@illuhad for testing

@gonzalobg gonzalobg changed the title Pack bodies in the System Draft: Pack bodies in the System Aug 13, 2024
@illuhad
Copy link
Collaborator

illuhad commented Aug 14, 2024

Tested it on AMD and NVIDIA:

RTX A5000:

Before

all-pairs (70k particles): 69.5ms/step
all-pairs-collapsed (70k particles): 24.9ms/step
octree(7M particles): 1735ms/step
bvh(7M particles): 1296ms/step

After

all-pairs (70k particles): 67.9ms/step
all-pairs-collapsed (70k particles): 25.1ms/step
octree(7M particles): 1696ms/step
bvh(7M particles): 1279ms/step

AMD Radeon Pro VII:

Before

all-pairs (70k particles): 221.3ms/step
bvh(7M particles): 6391ms/step

After

all-pairs (70k particles): 221.8 ms/step
bvh(7M particles): 6295ms/step

It seems to work, but the perf benefit is only very small (1-2%), even for all-pairs.

@gonzalobg
Copy link
Collaborator Author

Thanks, similar results on my end, negligible impact.
Should we merge this?

@illuhad
Copy link
Collaborator

illuhad commented Aug 14, 2024

I'm not sure. Do you feel like it simplifies or complicates code?

One thing I'm having a hard time understanding is why packing made such a big difference for the monopoles, but not for the particles, even in all-pairs which only relies on loading particles. Is it possible that perhaps something is not working right?

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.

2 participants