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

Windows artifact complains about vcomp140d.dll #631

Closed
tjaderxyz opened this issue Jun 14, 2020 · 15 comments
Closed

Windows artifact complains about vcomp140d.dll #631

tjaderxyz opened this issue Jun 14, 2020 · 15 comments
Labels

Comments

@tjaderxyz
Copy link

tjaderxyz commented Jun 14, 2020

I downloaded solvespace.exe from https://ci.appveyor.com/project/whitequark/solvespace/builds/33507638/artifacts and it is complaining about missing vcomp140d.dll.

@whitequark whitequark added the bug label Jun 14, 2020
@whitequark
Copy link
Contributor

For everyone else reading: this is a missing OpenMP library. I'll take a look at how can we make it statically linked.

@whitequark
Copy link
Contributor

@phkahler Well, I've discovered something awful: it is not possible to statically link the OpenMP runtime. That's true for MSVC, it is also true for Intel TBB, and it is true for gcc/glibc; I imaigne it would be true for MinGW, except MinGW ships without OpenMP.

Because our policy is that we ship strictly a single self-contained exe file, I'm afraid I'm going to have to roll back all of your OpenMP changes, as unfortunate as that is, as I see no other way to keep our usability promise. If it was possible to switch to MinGW or Clang for Windows builds and statically link OpenMP, I would consider doing that, even at the cost of losing compatibility with SpaceNavigator (or having to reverse-engineer their window messaging protocol), but I believe it isn't.

@whitequark
Copy link
Contributor

I guess the one viable alternative is to use C++17 parallel constructs. That's a pretty extreme jump in terms of requirements though.

@whitequark
Copy link
Contributor

It seems that MinGW build might have usable OpenMP, but this needs investigation.

@phkahler
Copy link
Member

Isnt that a debug build trying to use a debug version of the .dll? People really should not be running those.

I'm not certain if vcomp140.dll ships with windows by default, but even Autodesk has a page describing where to get it if users get errors about it.

@whitequark
Copy link
Contributor

I'm not certain if vcomp140.dll ships with windows by default

It doesn't.

even Autodesk has a page describing where to get it if users get errors about it.

Well, we should do better than Autodesk. But also we historically did better than Autodesk, and I see no reason to regress on usability now. In the past @jwesthues has expressed a strong desire to keep the current distribution model as it is (we were talking about CJK fonts then), and I very much agree.

@whitequark
Copy link
Contributor

Isnt that a debug build trying to use a debug version of the .dll? People really should not be running those.

Yeah, once #618 lands people will have access to release versions of nightlies, which will solve a lot of problems.

@phkahler
Copy link
Member

If it comes down to it, I would ask/suggest that the changes be kept in the code and just dont build with OpenMP. Whenever a suitable solution is found then searching for #pragma omp will show the known places that are safe to implement it.

@whitequark
Copy link
Contributor

That works for me. But I was more thinking along the lines of using another solution instead of OpenMP, such as std::thread.

@phkahler
Copy link
Member

@whitequark using std::thread should be fine, I just didn't want the improvements to be rolled back and lost.

@whitequark
Copy link
Contributor

@phkahler Sorry, I was unclear--by "roll back" I mean "stop shipping in default builds".

@whitequark
Copy link
Contributor

whitequark commented Jun 19, 2020

@phkahler Do you think we could use std::async instead of OMP? That one is in C++11.

@phkahler
Copy link
Member

@whitequark This is a bit outside my territory. I chose OpenMP primarily because I had used it many many years ago and it behaves exactly the way we want here (and I didn't even use any explicit options for partitioning the loop iterations among threads). This SO discussion brings up some possible issues with std::async:

https://stackoverflow.com/questions/47895048/c-async-vs-openmp-tasks

And it also links to this one:

https://stackoverflow.com/questions/14351352/does-asynclaunchasync-in-c11-make-thread-pools-obsolete-for-avoiding-expen/14351457#14351457

Those are interesting but don't seem to offer a definitive answer, just clarification of the possible issues.

The ideal construct is a thread pool dividing up the work. The idea of creating a thread per surface in SolveSpace seems like it could be bad, but that would need some testing. We could pass each thread a range of surfaces to process, but then we have to decide how many threads etc...

Is @rpavlik around? He seems to be quite knowledgeable about C++ constructs.

whitequark added a commit that referenced this issue Jun 21, 2020
This is unfortunate, but OpenMP is not available on some toolchains
(most MinGW builds) and, more importantly, breaks the distribution
model we use on Windows, which is a single self-contained executable.

Leave the OpenMP disabled by default everywhere so that we don't have
any second-class platforms where SolveSpace is slower than on first-
class ones.

Fixes #631.
@rpavlik
Copy link
Contributor

rpavlik commented Jun 22, 2020

Unfortunately there's nothing in C++ that maps neatly to openmp's parallel for in terms of simplicity.

@whitequark
Copy link
Contributor

That's unfortunate. The other solution (that preserves OpenMP) I can think of is using MinGW for the official Windows binaries. That will break the SpaceNavigator support, sadly, and I'm yet to see a MinGW build with working OpenMP (but I've been reassured that these exist).

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

No branches or pull requests

4 participants