Skip to content

Conversation

@tylercorleone
Copy link

@tylercorleone tylercorleone commented Nov 11, 2021

Hi @bchevalier, I'm Paolo Lupo, this is my personal/hobby account :)

Talking about your PR liabru#533 I had some doubts about the need to define variables like: positionAX = positionA.x, normalX = normal.x etc.

Initially I thought this was part of the "trick", necessary to improve the performance some more, but I didn't like the idea (I'm a Java/C/C++ developer, I'm not that confident with Javascript).

Now... I don't know why you choose to follow that approach, if for a personal coding style or to follow liabru's example like bodyA = collision.parentA (I think he made this just for readability) or something else.

I removed some of those variables, and the performances didn't change (actually the get a little better but I think it's a random thing):

Stress example on chrome:

original Resolver.solveVelocity (4800 samples): 1027 ms (18.5%)
new Resolver.solveVelocity (4800 samples): 640 ms (13.1%)
new Resolver.solveVelocity simplified (4800 samples): 662 ms (12.8%)
original Resolver.solveVelocity (4800 samples): 961 ms (19.0%)
new Resolver.solveVelocity (4800 samples): 622 ms (12.5%)
new Resolver.solveVelocity simplified (4800 samples): 578 ms (12.1%)

Complete logs:

simplified.txt

We saved 27 lines of code, but especially the code is more readable (less line of code used just to declare some aliases).

What do you think?

@bchevalier
Copy link
Owner

bchevalier commented Nov 11, 2021

Hey @tylercorleone,
Well I think the changes you made could make sense if you assume that functions won't be de-optimized by object modification at run time.

So for instance in the following code:

                var contact = pair.activeContacts[j],
                    offsetAX = contact.vertex.x - bodyA.position.x,
                    offsetAY = contact.vertex.y - bodyA.position.y,
                    offsetBX = contact.vertex.x - bodyB.position.x,
                    offsetBY = contact.vertex.y - bodyB.position.y,

Accessing the x property of the vertex property of contact could possibly made a single memory access with no condition on the types of contact or vertex. But due to the nature of JavaScript, if somewhere else in the code, the contact or vertex objects are added new properties dynamically. Then it would de-optimize this function even more in this simplified version (the reason for the de-optimization would be that internally the JS engine would have several type representations for vertex or contact and this would need to be tested at every property access).

So I guess what solution you will choose will depend on how sure you are that it won't happen.
Note that using typescript with very strict rules can almost ensure objects won't be de-optimized at run time. However I do not know any developer working with such strict rules that would avoid de-optimization completely and matterjs is not written in typescript.

@tylercorleone
Copy link
Author

Hi @bchevalier,

by "if somewhere else in the code, the contact or vertex objects are added new properties dynamically" you mean inside the context of that function call (e.g. inside a nested function or something similar) or before the function itself being called?

I'm thinking about a test to replicate this scenario. Adding a random property to the vertex and/or contact each time the function is being called could be a good example? I could add a new field between two access to the same property and compare it with this version. I'm very focused in code readability and simplicity, but of course I would put it on a second plane if this would impact the performance.

I'm a little concerned for the fact liabru not responding.

I've seen a comparison of the performance of several physical engines and Matter Js was the worst one, but it is also the most used and the one with the best API, IMO, so I would like to improve it.

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