Skip to content

Commit

Permalink
Remove the last use of memmove. NFC.
Browse files Browse the repository at this point in the history
Much clearer!
  • Loading branch information
whitequark committed May 23, 2019
1 parent fabffba commit cc10788
Showing 1 changed file with 2 additions and 4 deletions.
6 changes: 2 additions & 4 deletions src/mesh.cpp
Expand Up @@ -186,17 +186,15 @@ void SMesh::Simplify(int start) {
if(fabs(bDot) < LENGTH_EPS && fabs(dDot) < LENGTH_EPS) {
conv[WRAP((j+1), convc)] = c;
// and remove the vertex at j, which is a dup
memmove(conv+j, conv+j+1,
(convc - j - 1)*sizeof(conv[0]));
std::move(conv+j+1, conv+convc, conv+(convc-1));
convc--;
} else if(fabs(bDot) < LENGTH_EPS && dDot > 0) {
conv[j] = c;
} else if(fabs(dDot) < LENGTH_EPS && bDot > 0) {
conv[WRAP((j+1), convc)] = c;
} else if(bDot > 0 && dDot > 0) {
// conv[j] is unchanged, conv[j+1] goes to [j+2]
memmove(conv+j+2, conv+j+1,
(convc - j - 1)*sizeof(conv[0]));
std::move_backward(conv+j+1, conv+convc, conv+j+2);
conv[j+1] = c;
convc++;
} else {
Expand Down

7 comments on commit cc10788

@Evil-Spirit
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably, performance degradation

@whitequark
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

@Evil-Spirit
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember such performance issue when profiling. Evil-Spirit@44eb1a4#diff-b7a4b1c7a3c5a4ea4f00130583590ff4R336

@Evil-Spirit
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I turn back memmove and probably get some speedup.

@whitequark
Copy link
Contributor Author

@whitequark whitequark commented on cc10788 May 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, that's good to know. Maybe we need to define our own std::move for POD types that will have the same interface but is faster.

Though I do not know why they couldn't just specialize std::move itself for POD types, you can use SFINAE for it, I think...

@rpavlik
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I was pretty sure that std::move/std::copy is generally implemented in terms of memmove/memcpy for trivially-copyable stuff, etc. In godbolt a simple test of std::move in a vector of pod types actually turned into a bunch of MOVUPS if the range was of constant size, or a call to memmove if it wasn't: https://godbolt.org/z/t81plU

Please sign in to comment.