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

Support unity build. #6295

Merged
merged 3 commits into from
Oct 28, 2020
Merged

Support unity build. #6295

merged 3 commits into from
Oct 28, 2020

Conversation

trivialfis
Copy link
Member

@trivialfis trivialfis commented Oct 27, 2020

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 .

@trivialfis
Copy link
Member Author

trivialfis commented Oct 28, 2020

@hcho3 I tried enabling it on Windows CI, see if it helps. ;-)

@hcho3
Copy link
Collaborator

hcho3 commented Oct 28, 2020

@trivialfis Thanks. We should also try to get Ninja build working (#5832) to bring the build time under 10 min.

@trivialfis
Copy link
Member Author

@hcho3 It's under 10 minutes now.

@hcho3
Copy link
Collaborator

hcho3 commented Oct 28, 2020

Interesting. So unity build does help quite a bit on the Windows platform. Let me approve this PR.

@trivialfis
Copy link
Member Author

Em, it breaks external memory on Windows somehow.

@hcho3
Copy link
Collaborator

hcho3 commented Oct 28, 2020

@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)

@hcho3
Copy link
Collaborator

hcho3 commented Oct 28, 2020

Or can we drop support for external memory on the Windows platform?

@trivialfis
Copy link
Member Author

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.

drop support for external memory on the Windows platform

Dropping it won't solve the root cause, just hiding the issue.

@hcho3
Copy link
Collaborator

hcho3 commented Oct 28, 2020

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.

@trivialfis
Copy link
Member Author

@hcho3 Can I merge it by reverting the CI change?

@hcho3
Copy link
Collaborator

hcho3 commented Oct 28, 2020

Sure

@trivialfis
Copy link
Member Author

@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

@hcho3 hcho3 merged commit c4da967 into dmlc:master Oct 28, 2020
@trivialfis trivialfis deleted the unity-build branch October 28, 2020 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support building with cmake with unity build (build speedup)
2 participants