-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
Support unity build. #6295
Support unity build. #6295
Conversation
c8ed570
to
8345cb8
Compare
@hcho3 I tried enabling it on Windows CI, see if it helps. ;-) |
@trivialfis Thanks. We should also try to get Ninja build working (#5832) to bring the build time under 10 min. |
@hcho3 It's under 10 minutes now. |
Interesting. So unity build does help quite a bit on the Windows platform. Let me approve this PR. |
Em, it breaks external memory on Windows somehow. |
@trivialfis Another reason to extinguish race condition and other undefined behaviors from the external memory. This is the same issue as the Ninja PR (#5832) |
Or can we drop support for external memory on the Windows platform? |
It's not that easy with external memory. On the on hand I want to remove all use of threads for loading data, on the other hand the performance will be miserable without pre-fetching.
Dropping it won't solve the root cause, just hiding the issue. |
I agree. I just wanted to unblock this PR and #5832 so that we can start enjoying faster turn-around time. |
@hcho3 Can I merge it by reverting the CI change? |
Sure |
This reverts commit 8345cb8.
@hcho3 Do you think it's suitable to merge without passing power test? It seems travis is having difficulty on allocating power node. https://travis-ci.org/github/dmlc/xgboost/pull_requests |
See #6293 for details. The speedup may help some build machines for distribution, but I don't think it's useful for daily development. XGBoost's CPU compilation time on my machine is usually within a minute so not much room for improvement.
Update: Will be enabled on Windows CI.
Close #6293 .