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

Fix pathologically slow translate groups on Linux #973

Merged
merged 1 commit into from Mar 27, 2021

Conversation

pjanx
Copy link
Contributor

@pjanx pjanx commented Mar 27, 2021

Major performance improvement on GCC with libstdc++, see my comments in #939. Relates to discussion in #932.

All tests passed, nothing interesting in memcheck showed up.

I think this belongs in 3.0 and is obvious enough.

I've tried to use std::move_backwards but it didn't look better.

Major performance improvement on GCC with libstdc++.
@CLAassistant
Copy link

CLAassistant commented Mar 27, 2021

CLA assistant check
All committers have signed the CLA.

@phkahler phkahler merged commit 5e42275 into solvespace:master Mar 27, 2021
@ruevs
Copy link
Member

ruevs commented Mar 27, 2021

Ohhhh I can't wait go get to a PC to try this!

@ruevs
Copy link
Member

ruevs commented Mar 29, 2021

OK I tried it - on this model #932 (comment) the time to set the last translate group to 6 copies is:

  • 21 seconds before this patch
  • 15 seconds after it.

@ruevs
Copy link
Member

ruevs commented Mar 29, 2021

By the way the hot path is still moving all the entities "one down" from the insertion point.

This is at 10000 samples/s so a full 12.7 seconds of the 15 total is spent there.

20210329_ProfilingAfter973

@pjanx
Copy link
Contributor Author

pjanx commented Mar 29, 2021

Yes, it is still doing a lot. The inplace_merge was just doing something barely comprehensible, as I saw in Debug callgrind output.

Just now I can make something as simple as a keyboard case without tearing my hair out. :)

Perhaps a different data structure entirely would be appropriate, but I don't have any obvious favourites.

The other thing that came to my mind was postponing the sort, which looked complicated.

@rpavlik
Copy link
Contributor

rpavlik commented Mar 30, 2021

Wow, I think I touched that last before you. Blows my mind these are different performances, I would have expected nearly equivalent assembly after the optimizer had its way.

@phkahler
Copy link
Member

@rpavlik it seems the people on the C++ committee have a new definition of "in-place". It needs to allocate extra space, can take O(N logN) time, and uses custom comparator. No idea why it would take worse than linear time give the need to allocate more space. Seem like a good function to avoid.

@rpavlik
Copy link
Contributor

rpavlik commented Mar 30, 2021

Could we just do an insert instead of manually moving things?

@ruevs ruevs mentioned this pull request Mar 30, 2021
@ruevs
Copy link
Member

ruevs commented Mar 30, 2021

Could we just do an insert instead of manually moving things?

I'll try it. Anyway I am deep into it #939 (comment) But IdList is not quite a container (in the stl sense).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants