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

Question/Suggestion Parallel tesselation. #481

Closed
phkahler opened this issue Sep 11, 2019 · 7 comments · Fixed by #600
Closed

Question/Suggestion Parallel tesselation. #481

phkahler opened this issue Sep 11, 2019 · 7 comments · Fixed by #600

Comments

@phkahler
Copy link
Member

phkahler commented Sep 11, 2019

It looks like converting surfaces to triangles for display can take a lot of time. SolveSpace doesn't make use of multi-core CPUs as far as I can tell. One place we might try this is in the amazingly short function:

void SShell::TriangulateInto(SMesh *sm) {

    SSurface *s;
    for(s = surface.First(); s; s = surface.NextAfter(s)) {
        s->TriangulateInto(this, sm);
    }
}

All shells will have multiple surfaces. I'm not sure that function call can be done in parallel. Maybe we could have a look. If so, we'd just create a vector of triangle lists and use OpenMP parallel for (or equivalent?) to go over handing each surface its own list to populate and then concatenate the triangle lists after.

This hinges on weather that is a pure function. If so, what's the preferred way to do things in parallel? I'm not aware of any place SS does that.

@whitequark
Copy link
Contributor

We'd have to investigate what sort of parallel constructs are available (MSVC looks like it could be the problem here), but it's an option.

@baryluk
Copy link

baryluk commented Jan 13, 2020

C++17 std::for_each with std::execution:par is probably the best choice. It looks to be available in MSVC too.

@whitequark
Copy link
Contributor

It'd require bumping our MSVC version which historically has been a problem. Definitely solvable, just needs time and effort.

@phkahler
Copy link
Member Author

@baryluk if you wanted to prove the concept and report back any performance improvement, that would be awesome. We can decide on which parallel construct is best, or what MSVC version may be needed once there is a demonstrated improvement.

@phkahler
Copy link
Member Author

This is not parallel, but it does confirm that it could be run that way. Not sure what the best construct is to use, but tm could be a list/vector/etc of SSmesh objects whose count is known from surface.l.length or something like that. Not the best way to combine the lists either.

`
void SShell::TriangulateInto(SMesh *sm) {

SSurface *s;

SMesh tm = {};

STriangle *st;

for(s = surface.First(); s; s = surface.NextAfter(s)) {

    s->TriangulateInto(this, &tm);

    for (st = tm.l.First(); st; st = tm.l.NextAfter(st)) {

        sm->AddTriangle(st);

    }

    tm.Clear();

}

}
`

That this may be a performance win is still a guess on my part, I haven't done any profiling. Make a large circle and then Lathe to make a torus. Drag either the center point of the circle or change the radius with a low curve tolerance and it will slow way down. Since there is nothing difficult (like booleans) going on I assume the time is being spent in triangulating the 16 surfaces of the torus.

@phkahler
Copy link
Member Author

phkahler commented May 7, 2020

This is working on my parallel branch. The changes are completely local to that one function and are transparent unless you compile with -fopenmp using GCC. Other platforms should be similarly easy.

Gain is modest with many complex surfaces. YMMV

@whitequark
Copy link
Contributor

Using OpenMP seems less invasive than C++ parallel for, since it gracefully degrades (or at least I think it's graceful) on our old MSVC version, making it much easier to merge this code without having to upgrade the compiler at the same time.

We'd have to use at least MSVC 2017 to get any advantage on Windows, which we probably should because shipping an inferior version on one platform (the most popular platform, I assume) isn't good.

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

Successfully merging a pull request may close this issue.

4 participants