Skip to content

Commit

Permalink
Reduce Vector::Element calls in SKdNode::SnapToVertex. NFC.
Browse files Browse the repository at this point in the history
  • Loading branch information
@rpavlik @whitequark
rpavlik authored and whitequark committed May 21, 2019
1 parent f885daf commit 43c9cba
Showing 1 changed file with 10 additions and 6 deletions.
16 changes: 10 additions & 6 deletions src/mesh.cpp
View file
Expand Up @@ -580,16 +580,20 @@ void SKdNode::SnapToVertex(Vector v, SMesh *extras) {
bool mightHit = true;

for(k = 0; k < 3; k++) {
if((tr->a).Element(k) < v.Element(k) - KDTREE_EPS &&
(tr->b).Element(k) < v.Element(k) - KDTREE_EPS &&
(tr->c).Element(k) < v.Element(k) - KDTREE_EPS)
double trA = (tr->a).Element(k);
double trB = (tr->b).Element(k);
double trC = (tr->c).Element(k);

This comment has been minimized.

Sign in to view

Copy link
@Evil-Spirit

Evil-Spirit May 24, 2019 •
edited

Collaborator

Here we have to take all the elements before. Previous implementation can be faster because of lazy calculation of boolean expression. If we are faster than previous implementation, then we can be faster even more.

This comment has been minimized.

Sign in to view

Copy link
@whitequark

whitequark May 24, 2019

Contributor

cc @rpavlik

This comment has been minimized.

Sign in to view

Copy link
@rpavlik

rpavlik May 24, 2019 •
edited

Author Contributor

At least before the element lookups got inlined, the function call overhead of those element calls way blew other things out of the water in my profiling of some painfully-slow manipulations. I admit I have not profiled recently, so it may be that the element lookup inlining fixed this. I'm not sure it's quite so simple as lazy-calculation, though, since it appears like at least GCC is doing some vectorization. When I see XMM I know I'm over my head, so I'll just post these here: https://godbolt.org/z/-gYinl You can see the affected part of the graph is near the top of both of these - the change I submitted does actually produce a more "regular" control flow or something? (shorter branches and maybe fewer, looks like) I have no idea what that does to performance, I'll trust your judgement.
image

This is what Radare (well, Cutter) made of the functions old:

pre

and new:

new

Rest assured I will share the pathological files that prompted this frantic profiling and optimization last January when I find them :)

This comment has been minimized.

Sign in to view

Copy link
@Evil-Spirit

Evil-Spirit May 25, 2019

Collaborator

Ok, thank you, now I see what you know what you doing.
double vk = v.Element(k);
if(trA < vk - KDTREE_EPS &&
trB < vk - KDTREE_EPS &&
trC < vk - KDTREE_EPS)
{
mightHit = false;
break;
}
if((tr->a).Element(k) > v.Element(k) + KDTREE_EPS &&
(tr->b).Element(k) > v.Element(k) + KDTREE_EPS &&
(tr->c).Element(k) > v.Element(k) + KDTREE_EPS)
if(trA > vk + KDTREE_EPS &&
trB > vk + KDTREE_EPS &&
trC > vk + KDTREE_EPS)
{
mightHit = false;
break;
Expand Down

0 comments on commit 43c9cba

Please sign in to comment.