Skip to content

Fix pathologically slow translate groups on Linux#973

Merged
phkahler merged 1 commit into
solvespace:masterfrom
pjanx:master
Mar 27, 2021
Merged

Fix pathologically slow translate groups on Linux#973
phkahler merged 1 commit into
solvespace:masterfrom
pjanx:master

Conversation

@pjanx

@pjanx pjanx commented Mar 27, 2021

Copy link
Copy Markdown
Contributor

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

CLAassistant commented Mar 27, 2021

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

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

ruevs commented Mar 27, 2021

Copy link
Copy Markdown
Member

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

@ruevs

ruevs commented Mar 29, 2021

Copy link
Copy Markdown
Member

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

ruevs commented Mar 29, 2021

Copy link
Copy Markdown
Member

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

pjanx commented Mar 29, 2021

Copy link
Copy Markdown
Contributor Author

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

rpavlik commented Mar 30, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

rpavlik commented Mar 30, 2021

Copy link
Copy Markdown
Contributor

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

@ruevs ruevs mentioned this pull request Mar 30, 2021
@ruevs

ruevs commented Mar 30, 2021

Copy link
Copy Markdown
Member

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.

5 participants