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

Algorithm optimization of Pair methods create and update #526

Merged
merged 7 commits into from
Jun 12, 2018

Conversation

bchevalier
Copy link
Contributor

@bchevalier bchevalier commented Nov 24, 2017

Changes

  • No more Contact class (smaller source code)
  • Every vertex has a contact point (increasing memory footprint)
  • Pairs do not need to keep a map of current contacts (reducing memory footprint)
  • Contact ids do not need to be generated anymore
  • List of active contacts of a Pair are resized only when necessary
  • collision property added to Pair object at instantiation time to avoid future de-optimization.
  • Pair instantiation stripped of unnecessary parameter initialization that happen in the update method.
  • Some pair parameters are now only updated upon collision.

Result

Before the optimization:
screen shot 2017-11-20 at 3 51 23 pm

After optimization:
screen shot 2017-11-20 at 5 12 00 pm

We can see that the Pair.update method went from 7% of total used resources down to 0.13%.
That's a 50x improvement!

Overall, the profiling I performed on multiple tests seem to indicate a 6~7% boost to the engine.

@photonstorm
Copy link

Just tried implementing this and I'm seeing better results as well. My original performance was Pair.update at 13.9% and then V8 must have optimized it internally as I got a second entry at only 6.5%. After making the change the same very heavy test Pair.update used only 4.7% total. So well worth keeping in.

@bchevalier
Copy link
Contributor Author

I am a bit surprised that you are only getting it down to 4.7%. All my tests suggest it is now well below 1% in every profile I make.

@liabru liabru merged commit 7220435 into liabru:master Jun 12, 2018
@liabru
Copy link
Owner

liabru commented Mar 10, 2020

I've found a few issues here, at least when combined with #527, that I've been unable to resolve so far. While I much appreciate this work and potential improvements, unfortunately for the moment I've reverted this PR on master until we can find some way to fix.

It seems that combining the contacts into a single contact in this way appears to lower the quality of the result just a little too much. Maybe it can be solved, but it's not clear how.

I've completed some new work on tooling for comparing changes to the engine, discussed in PR #794, which I've used to confirm some stability and behaviour differences.

For those interested to see the issues I'm talking about, check out master on a commit before the revert that includes the comparison tool, e.g. fcdb4fa and run the dev server.

Go to http://localhost:8000/?compare#friction in the browser and wait for the box at the bottom to slide down and hit the wall.

friction-issue

The box bounces back a little in this version (shown in white), but not in the previous release version (overlayed here in gray). I know it seems like a minor detail, but the box should not bounce in this case as it has 0 restitution.

Also on http://localhost:8000/?compare#stress (and stress2), try collapse the stack by lifting a block on the top row up and left a little.

stack-issue

This version collapses much faster with a lot of bodies overlapping. The previous release version appears to hold up much better. This could just be resulting from a slight difference growing into a bigger one, but I can reproduce it every time in different ways. The testing tool also indicates this as an increase in average overlap.

I'm not sure whether #527 contributes more to this issue, but it appears that both need some review, hope that makes sense.

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.

3 participants