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

Filter operation reodered for a little performance boost #98

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

TrustNoOneElse
Copy link
Contributor

Hi, i have huge performance issues due to the method penetration_constraints. I will try to investigate further in this field to get the most of performance out, but im still learning, so making progress is especially for me hard.

I made this little change and thought maybe it is useful so i try a PR ^^ And it at least helped me to support a little more collider amount then before. If its non sense, then feel free to correct me and close this :) Anyway thanks for reading.

@TrustNoOneElse TrustNoOneElse changed the title filter operation reodered for a little performance boost Filter operation reodered for a little performance boost Jul 26, 2023
@Jondolf Jondolf added C-Enhancement New feature or request C-Performance Improvements or questions related to performance labels Jul 26, 2023
@Jondolf
Copy link
Owner

Jondolf commented Jul 26, 2023

Thanks, it looks like this might give a ~2% performance boost according to the cubes benchmark. Maybe for other cases it's more significant :)

I'm not sure if we even need that filter at all since the penetration constraints check the depth anyway, but iirc it had some problems so I left it there for now.

I believe the biggest bottleneck is the actual narrow phase collision detection using Parry (the call of compute_contacts in penetration_constraints). It's currently single-threaded and annoyingly coupled with the penetration constraints. Ideally, we would move it into a separate NarrowPhasePlugin that would process all broad phase collision pairs in parallel, but separating the narrow phase like this caused significant instability when I last tried it. It's definitely the biggest performance problem though so we should look into it more.

Eventually, a more optimized broad phase algorithm and a parallel constraint solver would also be good to have, but these are probably future concerns

@Jondolf Jondolf merged commit 6e294ed into Jondolf:main Jul 26, 2023
@TrustNoOneElse
Copy link
Contributor Author

Thanks, it looks like this might give a ~2% performance boost according to the cubes benchmark. Maybe for other cases it's more significant :)

I'm not sure if we even need that filter at all since the penetration constraints check the depth anyway, but iirc it had some problems so I left it there for now.

I believe the biggest bottleneck is the actual narrow phase collision detection using Parry (the call of compute_contacts in penetration_constraints). It's currently single-threaded and annoyingly coupled with the penetration constraints. Ideally, we would move it into a separate NarrowPhasePlugin that would process all broad phase collision pairs in parallel, but separating the narrow phase like this caused significant instability when I last tried it. It's definitely the biggest performance problem though so we should look into it more.

Eventually, a more optimized broad phase algorithm and a parallel constraint solver would also be good to have, but these are probably future concerns

Thanks for the insights! I can support your belive with tracy, that the method almost takes like 55% of the time, the next heavy thing compared to this was the second query iteration, the get_many_mut accordingly to the stats. Im not sure how good i used tracy. But i will look in that direction that you said and will try some experiments with it. Thank you very much :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Enhancement New feature or request C-Performance Improvements or questions related to performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants