From 97f960f639ac56dea2f0d8ac923771f35ea11ba0 Mon Sep 17 00:00:00 2001 From: fis Date: Mon, 15 Jul 2019 03:04:09 -0400 Subject: [PATCH 01/12] Use CMake config file for representing version. * Generate c and Python version file with CMake. The generated file is written into source tree. But unless XGBoost upgrades its version, there will be no actual modification. This retains compatibility with Makefiles for R. * Add XGBoost version the DMatrix binaries. --- CMakeLists.txt | 8 ++------ cmake/Python_version.in | 1 + cmake/Version.cmake | 10 ++++++++++ cmake/build_config.h.in | 28 ++++++++++++++++++++++++++++ include/xgboost/base.h | 2 ++ include/xgboost/build_config.h | 6 ++++++ src/data/data.cc | 26 +++++++++++++++++--------- tests/cpp/data/test_metainfo.cc | 18 +++++++++++------- 8 files changed, 77 insertions(+), 22 deletions(-) create mode 100644 cmake/Python_version.in create mode 100644 cmake/Version.cmake create mode 100644 cmake/build_config.h.in diff --git a/CMakeLists.txt b/CMakeLists.txt index 2ffff89f5a29..4fd8c7159fa1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -13,12 +13,8 @@ if (CMAKE_COMPILER_IS_GNUCC AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 5.0) message(FATAL_ERROR "GCC version must be at least 5.0!") endif() -message(STATUS "xgboost VERSION: ${xgboost_VERSION}") -set(XGBOOST_DEFINITIONS - ${XGBOOST_DEFINITIONS} - -DXGBOOST_VER_MAJOR=${xgboost_VERSION_MAJOR} - -DXGBOOST_VER_MINOR=${xgboost_VERSION_MINOR} - -DXGBOOST_VER_PATCH=${xgboost_VERSION_PATCH}) +include(${xgboost_SOURCE_DIR}/cmake/Version.cmake) +write_version() set_default_configuration_release() #-- Options diff --git a/cmake/Python_version.in b/cmake/Python_version.in new file mode 100644 index 000000000000..219b1d74af53 --- /dev/null +++ b/cmake/Python_version.in @@ -0,0 +1 @@ +@xgboost_VERSION_MAJOR@.@xgboost_VERSION_MINOR@.@xgboost_VERSION_PATCH@-SNAPSHOT \ No newline at end of file diff --git a/cmake/Version.cmake b/cmake/Version.cmake new file mode 100644 index 000000000000..c8eafc8f1b51 --- /dev/null +++ b/cmake/Version.cmake @@ -0,0 +1,10 @@ +function (write_version) + message(STATUS "xgboost VERSION: ${xgboost_VERSION}") + configure_file( + ${xgboost_SOURCE_DIR}/cmake/build_config.h.in + ${xgboost_SOURCE_DIR}/include/xgboost/build_config.h @ONLY) + configure_file( + ${xgboost_SOURCE_DIR}/cmake/Python_version.in + ${xgboost_SOURCE_DIR}/python-package/xgboost/VERSION + ) +endfunction (write_version) diff --git a/cmake/build_config.h.in b/cmake/build_config.h.in new file mode 100644 index 000000000000..3bfab88dc59b --- /dev/null +++ b/cmake/build_config.h.in @@ -0,0 +1,28 @@ +/*! + * Copyright 2019 by Contributors + * \file build_config.h + * + * Generated from `cmake/build_config.h.in` by cmake. + */ +#ifndef XGBOOST_BUILD_CONFIG_H_ +#define XGBOOST_BUILD_CONFIG_H_ + +// These check are for Makefile. +#if !defined(XGBOOST_MM_PREFETCH_PRESENT) && !defined(XGBOOST_BUILTIN_PREFETCH_PRESENT) +/* default logic for software pre-fetching */ +#if (defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_AMD64))) || defined(__INTEL_COMPILER) +// Enable _mm_prefetch for Intel compiler and MSVC+x86 + #define XGBOOST_MM_PREFETCH_PRESENT + #define XGBOOST_BUILTIN_PREFETCH_PRESENT +#elif defined(__GNUC__) +// Enable __builtin_prefetch for GCC +#define XGBOOST_BUILTIN_PREFETCH_PRESENT +#endif // GUARDS + +#endif // !defined(XGBOOST_MM_PREFETCH_PRESENT) && !defined() + +#define XGBOOST_VER_MAJOR @xgboost_VERSION_MAJOR@ +#define XGBOOST_VER_MINOR @xgboost_VERSION_MINOR@ +#define XGBOOST_VER_PATCH @xgboost_VERSION_PATCH@ + +#endif // XGBOOST_BUILD_CONFIG_H_ diff --git a/include/xgboost/base.h b/include/xgboost/base.h index 0922cb22e73c..673bd633ff9c 100644 --- a/include/xgboost/base.h +++ b/include/xgboost/base.h @@ -217,6 +217,8 @@ const bst_float kRtEps = 1e-6f; using omp_ulong = dmlc::omp_ulong; // NOLINT /*! \brief define unsigned int for openmp loop */ using bst_omp_uint = dmlc::omp_uint; // NOLINT +/*! \brief Type used for representing version number in binary form.*/ +using XGBoostVersionT = int32_t; /*! * \brief define compatible keywords in g++ diff --git a/include/xgboost/build_config.h b/include/xgboost/build_config.h index 6d364a6ff081..f626f390a2c7 100644 --- a/include/xgboost/build_config.h +++ b/include/xgboost/build_config.h @@ -1,6 +1,8 @@ /*! * Copyright 2019 by Contributors * \file build_config.h + * + * Generated from `cmake/build_config.h.in` by cmake. */ #ifndef XGBOOST_BUILD_CONFIG_H_ #define XGBOOST_BUILD_CONFIG_H_ @@ -19,4 +21,8 @@ #endif // !defined(XGBOOST_MM_PREFETCH_PRESENT) && !defined() +#define XGBOOST_VER_MAJOR 1 +#define XGBOOST_VER_MINOR 0 +#define XGBOOST_VER_PATCH 0 + #endif // XGBOOST_BUILD_CONFIG_H_ diff --git a/src/data/data.cc b/src/data/data.cc index 9f30885310cb..35aff80ba900 100644 --- a/src/data/data.cc +++ b/src/data/data.cc @@ -4,6 +4,7 @@ */ #include #include +#include #include #include @@ -34,8 +35,12 @@ void MetaInfo::Clear() { } void MetaInfo::SaveBinary(dmlc::Stream *fo) const { - int32_t version = kVersion; - fo->Write(&version, sizeof(version)); + XGBoostVersionT major{XGBOOST_VER_MAJOR}; + XGBoostVersionT minor{XGBOOST_VER_MINOR}; + XGBoostVersionT patch{XGBOOST_VER_PATCH}; + fo->Write(&major, sizeof(major)); + fo->Write(&minor, sizeof(minor)); + fo->Write(&patch, sizeof(patch)); fo->Write(&num_row_, sizeof(num_row_)); fo->Write(&num_col_, sizeof(num_col_)); fo->Write(&num_nonzero_, sizeof(num_nonzero_)); @@ -47,19 +52,22 @@ void MetaInfo::SaveBinary(dmlc::Stream *fo) const { } void MetaInfo::LoadBinary(dmlc::Stream *fi) { - int version; - CHECK(fi->Read(&version, sizeof(version)) == sizeof(version)) << "MetaInfo: invalid version"; - CHECK(version >= 1 && version <= kVersion) << "MetaInfo: unsupported file version"; + XGBoostVersionT major{0}, minor{0}, patch{0}; + CHECK(fi->Read(&major, sizeof(major)) == sizeof(major)); + CHECK(fi->Read(&minor, sizeof(major)) == sizeof(minor)); + CHECK(fi->Read(&patch, sizeof(major)) == sizeof(patch)); + + // MetaInfo is saved in `SparsePageSource'. So the version in MetaInfo represents the + // version of DMatrix. + CHECK_GE(major, 1) << "Binary DMatrix generated by XGBoost: " + << major << "." << minor << "." << patch << " is not supported"; CHECK(fi->Read(&num_row_, sizeof(num_row_)) == sizeof(num_row_)) << "MetaInfo: invalid format"; CHECK(fi->Read(&num_col_, sizeof(num_col_)) == sizeof(num_col_)) << "MetaInfo: invalid format"; CHECK(fi->Read(&num_nonzero_, sizeof(num_nonzero_)) == sizeof(num_nonzero_)) << "MetaInfo: invalid format"; CHECK(fi->Read(&labels_.HostVector())) << "MetaInfo: invalid format"; CHECK(fi->Read(&group_ptr_)) << "MetaInfo: invalid format"; - if (version == kVersionWithQid) { - std::vector qids; - CHECK(fi->Read(&qids)) << "MetaInfo: invalid format"; - } + CHECK(fi->Read(&weights_.HostVector())) << "MetaInfo: invalid format"; CHECK(fi->Read(&root_index_)) << "MetaInfo: invalid format"; CHECK(fi->Read(&base_margin_.HostVector())) << "MetaInfo: invalid format"; diff --git a/tests/cpp/data/test_metainfo.cc b/tests/cpp/data/test_metainfo.cc index 38e157fbb636..d6f14ab2624a 100644 --- a/tests/cpp/data/test_metainfo.cc +++ b/tests/cpp/data/test_metainfo.cc @@ -51,20 +51,24 @@ TEST(MetaInfo, SaveLoadBinary) { dmlc::TemporaryDirectory tempdir; const std::string tmp_file = tempdir.path + "/metainfo.binary"; - dmlc::Stream* fs = dmlc::Stream::Create(tmp_file.c_str(), "w"); - info.SaveBinary(fs); - delete fs; + { + std::unique_ptr fs { + dmlc::Stream::Create(tmp_file.c_str(), "w") + }; + info.SaveBinary(fs.get()); + } - ASSERT_EQ(GetFileSize(tmp_file), 76) + ASSERT_EQ(GetFileSize(tmp_file), 84) << "Expected saved binary file size to be same as object size"; - fs = dmlc::Stream::Create(tmp_file.c_str(), "r"); + std::unique_ptr fs { + dmlc::Stream::Create(tmp_file.c_str(), "r") + }; xgboost::MetaInfo inforead; - inforead.LoadBinary(fs); + inforead.LoadBinary(fs.get()); EXPECT_EQ(inforead.labels_.HostVector(), info.labels_.HostVector()); EXPECT_EQ(inforead.num_col_, info.num_col_); EXPECT_EQ(inforead.num_row_, info.num_row_); - delete fs; } TEST(MetaInfo, LoadQid) { From 65a86b1d6d11d76915ce8f3e1edc8be547bf7f79 Mon Sep 17 00:00:00 2001 From: fis Date: Sat, 12 Oct 2019 10:05:57 -0400 Subject: [PATCH 02/12] Define a handler. --- include/xgboost/json.h | 10 +++++ src/c_api/c_api.cc | 6 +++ src/common/json.cc | 8 ++++ src/common/version.cc | 76 ++++++++++++++++++++++++++++++++ src/common/version.h | 35 +++++++++++++++ src/data/data.cc | 18 +++----- tests/cpp/common/test_version.cc | 35 +++++++++++++++ 7 files changed, 176 insertions(+), 12 deletions(-) create mode 100644 src/common/version.cc create mode 100644 src/common/version.h create mode 100644 tests/cpp/common/test_version.cc diff --git a/include/xgboost/json.h b/include/xgboost/json.h index 790eca8b8337..99ad88aa3feb 100644 --- a/include/xgboost/json.h +++ b/include/xgboost/json.h @@ -322,6 +322,9 @@ class Json { static void Dump(Json json, std::ostream* stream, bool pretty = ConsoleLogger::ShouldLog( ConsoleLogger::LogVerbosity::kDebug)); + static void Dump(Json json, std::string* out, + bool pretty = ConsoleLogger::ShouldLog( + ConsoleLogger::LogVerbosity::kDebug)); Json() : ptr_{new JsonNull} {} @@ -400,6 +403,13 @@ class Json { return *ptr_ == *(rhs.ptr_); } + friend std::ostream& operator<<(std::ostream& os, Json const& j) { + std::string str; + Json::Dump(j, &str); + os << str; + return os; + } + private: std::shared_ptr ptr_; }; diff --git a/src/c_api/c_api.cc b/src/c_api/c_api.cc index 75190adc8a64..11654f8b2b3d 100644 --- a/src/c_api/c_api.cc +++ b/src/c_api/c_api.cc @@ -149,6 +149,12 @@ struct XGBAPIThreadLocalEntry { std::vector tmp_gpair; }; +XGB_EXTERN_C void XGBoostVersion(int* major, int* minor, int* patch) { + *major = XGBOOST_VER_MAJOR; + *minor = XGBOOST_VER_MINOR; + *patch = XGBOOST_VER_PATCH; +} + // define the threadlocal store. using XGBAPIThreadLocalStore = dmlc::ThreadLocalStore; diff --git a/src/common/json.cc b/src/common/json.cc index 5c14db47b257..2c8000787c45 100644 --- a/src/common/json.cc +++ b/src/common/json.cc @@ -718,5 +718,13 @@ void Json::Dump(Json json, std::ostream *stream, bool pretty) { writer.Save(json); } +void Json::Dump(Json json, std::string* str, bool pretty) { + GlobalCLocale guard; + std::stringstream ss; + JsonWriter writer(&ss, pretty); + writer.Save(json); + *str = ss.str(); +} + Json& Json::operator=(Json const &other) = default; } // namespace xgboost diff --git a/src/common/version.cc b/src/common/version.cc new file mode 100644 index 000000000000..9b12460cc053 --- /dev/null +++ b/src/common/version.cc @@ -0,0 +1,76 @@ +/*! + * Copyright 2019 XGBoost contributors + */ +#include + +#include +#include + +#include "xgboost/logging.h" +#include "xgboost/json.h" +#include "version.h" + +namespace xgboost { + +constexpr Version::TripletT Version::kInvalid; + +Version::TripletT Version::Load(Json const& in, bool check) { + if (get(in).find("version") == get(in).cend()) { + return kInvalid; + } + Integer::Int major {0}, minor {0}, patch {0}; + try { + auto const& j_version = get(in["version"]); + std::tie(major, minor, patch) = std::make_tuple( + get(j_version.at(0)), + get(j_version.at(1)), + get(j_version.at(2))); + } catch (dmlc::Error const& e) { + LOG(FATAL) << "Invaid version format in loaded JSON object: " << in; + } + + return {major, minor, patch}; +} + +Version::TripletT Version::Load(dmlc::Stream* fi) { + XGBoostVersionT major{0}, minor{0}, patch{0}; + // This is only used in DMatrix serialization, so doesn't break model compability. + std::string msg { "Invalid version Format. Are you loading from binary file " + "that's generated by XGBoost < 1.0.0?" }; + CHECK_EQ(fi->Read(&major, sizeof(major)), sizeof(major)) << msg; + CHECK_EQ(fi->Read(&minor, sizeof(major)), sizeof(minor)) << msg; + CHECK_EQ(fi->Read(&patch, sizeof(major)), sizeof(patch)) << msg; + return {major, minor, patch}; +} + +void Version::Save(Json* out) { + Integer::Int major, minor, patch; + std::tie(major, minor, patch)= Self(); + (*out)["version"] = std::vector{Json(Integer{major}), + Json(Integer{minor}), + Json(Integer{patch})}; +} + +void Version::Save(dmlc::Stream* fo) { + XGBoostVersionT major, minor, patch; + std::tie(major, minor, patch) = Self(); + fo->Write(&major, sizeof(major)); + fo->Write(&minor, sizeof(minor)); + fo->Write(&patch, sizeof(patch)); +} + +std::string Version::String(TripletT const& version) { + std::stringstream ss; + ss << std::get<0>(version) << "." << get<1>(version) << "." << get<2>(version); + return ss.str(); +} + +Version::TripletT Version::Self() { + return {XGBOOST_VER_MAJOR, XGBOOST_VER_MINOR, XGBOOST_VER_PATCH}; +} + +bool Version::Same(TripletT const& triplet) { + return triplet == Self(); +} + +} // namespace xgboost diff --git a/src/common/version.h b/src/common/version.h new file mode 100644 index 000000000000..bf5dbfc14fc5 --- /dev/null +++ b/src/common/version.h @@ -0,0 +1,35 @@ +/*! + * Copyright 2019 XGBoost contributors + */ +#ifndef XGBOOST_COMMON_VERSION_H_ +#define XGBOOST_COMMON_VERSION_H_ + +#include +#include +#include + +#include "xgboost/base.h" + +namespace xgboost { +class Json; +// a static class for handling version info +struct Version { + using TripletT = std::tuple; + static constexpr TripletT kInvalid {-1, -1, -1}; + + // Save/Load version info to Json document + static TripletT Load(Json const& in, bool check = false); + static void Save(Json* out); + + // Save/Load version info to dmlc::Stream + static Version::TripletT Load(dmlc::Stream* fi); + static void Save(dmlc::Stream* fo); + + static std::string String(TripletT const& version); + static TripletT Self(); + + static bool Same(TripletT const& triplet); +}; + +} // namespace xgboost +#endif // XGBOOST_COMMON_VERSION_H_ diff --git a/src/data/data.cc b/src/data/data.cc index 35aff80ba900..5045901a0c89 100644 --- a/src/data/data.cc +++ b/src/data/data.cc @@ -12,6 +12,7 @@ #include "./simple_dmatrix.h" #include "./simple_csr_source.h" #include "../common/io.h" +#include "../common/version.h" #include "../common/group_data.h" #if DMLC_ENABLE_STD_THREAD @@ -35,12 +36,7 @@ void MetaInfo::Clear() { } void MetaInfo::SaveBinary(dmlc::Stream *fo) const { - XGBoostVersionT major{XGBOOST_VER_MAJOR}; - XGBoostVersionT minor{XGBOOST_VER_MINOR}; - XGBoostVersionT patch{XGBOOST_VER_PATCH}; - fo->Write(&major, sizeof(major)); - fo->Write(&minor, sizeof(minor)); - fo->Write(&patch, sizeof(patch)); + Version::Save(fo); fo->Write(&num_row_, sizeof(num_row_)); fo->Write(&num_col_, sizeof(num_col_)); fo->Write(&num_nonzero_, sizeof(num_nonzero_)); @@ -52,15 +48,13 @@ void MetaInfo::SaveBinary(dmlc::Stream *fo) const { } void MetaInfo::LoadBinary(dmlc::Stream *fi) { - XGBoostVersionT major{0}, minor{0}, patch{0}; - CHECK(fi->Read(&major, sizeof(major)) == sizeof(major)); - CHECK(fi->Read(&minor, sizeof(major)) == sizeof(minor)); - CHECK(fi->Read(&patch, sizeof(major)) == sizeof(patch)); - + auto version = Version::Load(fi); + auto major = std::get<0>(version); // MetaInfo is saved in `SparsePageSource'. So the version in MetaInfo represents the // version of DMatrix. CHECK_GE(major, 1) << "Binary DMatrix generated by XGBoost: " - << major << "." << minor << "." << patch << " is not supported"; + << Version::String(version) << " is no longer supported. " + << "Please process and save your data in current again."; CHECK(fi->Read(&num_row_, sizeof(num_row_)) == sizeof(num_row_)) << "MetaInfo: invalid format"; CHECK(fi->Read(&num_col_, sizeof(num_col_)) == sizeof(num_col_)) << "MetaInfo: invalid format"; CHECK(fi->Read(&num_nonzero_, sizeof(num_nonzero_)) == sizeof(num_nonzero_)) diff --git a/tests/cpp/common/test_version.cc b/tests/cpp/common/test_version.cc new file mode 100644 index 000000000000..9f3f42a0349f --- /dev/null +++ b/tests/cpp/common/test_version.cc @@ -0,0 +1,35 @@ +/*! + * Copyright 2019 XGBoost contributors + */ +#include + +#include +#include + +#include +#include + +#include "../../../src/common/version.h" + +namespace xgboost { +TEST(Version, IO) { + Json j_ver { Object() }; + Version::Save(&j_ver); + auto triplet { Version::Load(j_ver) }; + ASSERT_TRUE(Version::Same(triplet)); + + dmlc::TemporaryDirectory tempdir; + const std::string fname = tempdir.path + "/version"; + + { + std::unique_ptr fo(dmlc::Stream::Create(fname.c_str(), "w")); + Version::Save(fo.get()); + } + + { + std::unique_ptr fi(dmlc::Stream::Create(fname.c_str(), "r")); + auto triplet { Version::Load(fi.get())};; + ASSERT_TRUE(Version::Same(triplet)); + } +} +} // namespace xgboost From 292770e21a278178a013af97658348f3fcbabeb7 Mon Sep 17 00:00:00 2001 From: fis Date: Sat, 12 Oct 2019 11:14:56 -0400 Subject: [PATCH 03/12] Test for string. --- src/common/version.cc | 1 + tests/cpp/common/test_version.cc | 30 ++++++++++++++++++++++++++++-- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/src/common/version.cc b/src/common/version.cc index 9b12460cc053..17ef033040de 100644 --- a/src/common/version.cc +++ b/src/common/version.cc @@ -5,6 +5,7 @@ #include #include +#include #include "xgboost/logging.h" #include "xgboost/json.h" diff --git a/tests/cpp/common/test_version.cc b/tests/cpp/common/test_version.cc index 9f3f42a0349f..fbbef599272a 100644 --- a/tests/cpp/common/test_version.cc +++ b/tests/cpp/common/test_version.cc @@ -9,10 +9,12 @@ #include #include +#include + #include "../../../src/common/version.h" namespace xgboost { -TEST(Version, IO) { +TEST(Version, Basic) { Json j_ver { Object() }; Version::Save(&j_ver); auto triplet { Version::Load(j_ver) }; @@ -31,5 +33,29 @@ TEST(Version, IO) { auto triplet { Version::Load(fi.get())};; ASSERT_TRUE(Version::Same(triplet)); } + + std::string str { Version::String(triplet) }; + + size_t ptr {0}; + XGBoostVersionT v {0}; + v = std::stoi(str, &ptr); + ASSERT_EQ(str.at(ptr), '.'); + ASSERT_EQ(v, XGBOOST_VER_MAJOR) << "major: " << v; + + str = str.substr(ptr+1); + + ptr = 0; + v = std::stoi(str, &ptr); + ASSERT_EQ(str.at(ptr), '.'); + ASSERT_EQ(v, XGBOOST_VER_MINOR) << "minor: " << v;; + + str = str.substr(ptr+1); + + ptr = 0; + v = std::stoi(str, &ptr); + ASSERT_EQ(v, XGBOOST_VER_MINOR) << "patch: " << v;; + + str = str.substr(ptr); + ASSERT_EQ(str.size(), 0); } -} // namespace xgboost +} // namespace xgboost \ No newline at end of file From c38cb9f306c041aadd18609dd36942d089c1637d Mon Sep 17 00:00:00 2001 From: fis Date: Sat, 12 Oct 2019 11:21:53 -0400 Subject: [PATCH 04/12] Typo. --- src/data/data.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data/data.cc b/src/data/data.cc index 5045901a0c89..3e25b4eca84e 100644 --- a/src/data/data.cc +++ b/src/data/data.cc @@ -54,7 +54,7 @@ void MetaInfo::LoadBinary(dmlc::Stream *fi) { // version of DMatrix. CHECK_GE(major, 1) << "Binary DMatrix generated by XGBoost: " << Version::String(version) << " is no longer supported. " - << "Please process and save your data in current again."; + << "Please process and save your data in current version again."; CHECK(fi->Read(&num_row_, sizeof(num_row_)) == sizeof(num_row_)) << "MetaInfo: invalid format"; CHECK(fi->Read(&num_col_, sizeof(num_col_)) == sizeof(num_col_)) << "MetaInfo: invalid format"; CHECK(fi->Read(&num_nonzero_, sizeof(num_nonzero_)) == sizeof(num_nonzero_)) From 6ec88d1b120c4da303434ea12b379b763df3b3c9 Mon Sep 17 00:00:00 2001 From: fis Date: Sat, 12 Oct 2019 13:32:50 -0400 Subject: [PATCH 05/12] Use make tuple. --- src/common/version.cc | 8 ++++---- src/common/version.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/common/version.cc b/src/common/version.cc index 17ef033040de..a06e18f2fd99 100644 --- a/src/common/version.cc +++ b/src/common/version.cc @@ -13,7 +13,7 @@ namespace xgboost { -constexpr Version::TripletT Version::kInvalid; +const Version::TripletT Version::kInvalid {-1, -1, -1}; Version::TripletT Version::Load(Json const& in, bool check) { if (get(in).find("version") == get(in).cend()) { @@ -30,7 +30,7 @@ Version::TripletT Version::Load(Json const& in, bool check) { LOG(FATAL) << "Invaid version format in loaded JSON object: " << in; } - return {major, minor, patch}; + return std::make_tuple(major, minor, patch); } Version::TripletT Version::Load(dmlc::Stream* fi) { @@ -41,7 +41,7 @@ Version::TripletT Version::Load(dmlc::Stream* fi) { CHECK_EQ(fi->Read(&major, sizeof(major)), sizeof(major)) << msg; CHECK_EQ(fi->Read(&minor, sizeof(major)), sizeof(minor)) << msg; CHECK_EQ(fi->Read(&patch, sizeof(major)), sizeof(patch)) << msg; - return {major, minor, patch}; + return std::make_tuple(major, minor, patch); } void Version::Save(Json* out) { @@ -67,7 +67,7 @@ std::string Version::String(TripletT const& version) { } Version::TripletT Version::Self() { - return {XGBOOST_VER_MAJOR, XGBOOST_VER_MINOR, XGBOOST_VER_PATCH}; + return std::make_tuple(XGBOOST_VER_MAJOR, XGBOOST_VER_MINOR, XGBOOST_VER_PATCH); } bool Version::Same(TripletT const& triplet) { diff --git a/src/common/version.h b/src/common/version.h index bf5dbfc14fc5..96885f6a8a82 100644 --- a/src/common/version.h +++ b/src/common/version.h @@ -15,7 +15,7 @@ class Json; // a static class for handling version info struct Version { using TripletT = std::tuple; - static constexpr TripletT kInvalid {-1, -1, -1}; + static const TripletT kInvalid; // Save/Load version info to Json document static TripletT Load(Json const& in, bool check = false); From 900989afe456ab1d87ef62d8ab463829f92470d0 Mon Sep 17 00:00:00 2001 From: fis Date: Sat, 12 Oct 2019 13:54:52 -0400 Subject: [PATCH 06/12] Add c doc for version. --- include/xgboost/c_api.h | 7 +++++++ src/c_api/c_api.cc | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/include/xgboost/c_api.h b/include/xgboost/c_api.h index 39f6811d804f..9466ee207700 100644 --- a/include/xgboost/c_api.h +++ b/include/xgboost/c_api.h @@ -58,6 +58,13 @@ typedef struct { // NOLINT(*) float* value; } XGBoostBatchCSR; +/*! + * \brief Return the version of the XGBoost library being currently used. + * \param major Store the major version number + * \param minor Store the minor version number + * \param patch Store the patch (revision) number + */ +XGB_DLL void XGBoostVersion(int* major, int* minor, int* patch); /*! * \brief Callback to set the data to handle, diff --git a/src/c_api/c_api.cc b/src/c_api/c_api.cc index 11654f8b2b3d..a8370b045be6 100644 --- a/src/c_api/c_api.cc +++ b/src/c_api/c_api.cc @@ -149,7 +149,7 @@ struct XGBAPIThreadLocalEntry { std::vector tmp_gpair; }; -XGB_EXTERN_C void XGBoostVersion(int* major, int* minor, int* patch) { +XGB_DLL void XGBoostVersion(int* major, int* minor, int* patch) { *major = XGBOOST_VER_MAJOR; *minor = XGBOOST_VER_MINOR; *patch = XGBOOST_VER_PATCH; From 42c93cb961fe77a62def55c71ba00f72092304fc Mon Sep 17 00:00:00 2001 From: fis Date: Sun, 13 Oct 2019 00:02:37 -0400 Subject: [PATCH 07/12] Amalgamation. --- amalgamation/xgboost-all0.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/amalgamation/xgboost-all0.cc b/amalgamation/xgboost-all0.cc index b7712bd3944c..2c528d13a80f 100644 --- a/amalgamation/xgboost-all0.cc +++ b/amalgamation/xgboost-all0.cc @@ -69,6 +69,7 @@ #include "../src/common/hist_util.cc" #include "../src/common/json.cc" #include "../src/common/io.cc" +#include "../src/common/version.cc" // c_api #include "../src/c_api/c_api.cc" From b106427fc0b38d02096521c728b96aa600e2fa30 Mon Sep 17 00:00:00 2001 From: fis Date: Mon, 14 Oct 2019 10:14:24 -0400 Subject: [PATCH 08/12] Remove old version info in DMatrix. --- include/xgboost/data.h | 4 ---- src/data/data.cc | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/include/xgboost/data.h b/include/xgboost/data.h index b8bce91de588..44d9ea841d59 100644 --- a/include/xgboost/data.h +++ b/include/xgboost/data.h @@ -66,10 +66,6 @@ class MetaInfo { * can be used to specify initial prediction to boost from. */ HostDeviceVector base_margin_; - /*! \brief version flag, used to check version of this info */ - static const int kVersion = 3; - /*! \brief version that contains qid field */ - static const int kVersionWithQid = 2; /*! \brief default constructor */ MetaInfo() = default; /*! diff --git a/src/data/data.cc b/src/data/data.cc index 3e25b4eca84e..8cf37bff6e18 100644 --- a/src/data/data.cc +++ b/src/data/data.cc @@ -52,7 +52,7 @@ void MetaInfo::LoadBinary(dmlc::Stream *fi) { auto major = std::get<0>(version); // MetaInfo is saved in `SparsePageSource'. So the version in MetaInfo represents the // version of DMatrix. - CHECK_GE(major, 1) << "Binary DMatrix generated by XGBoost: " + CHECK_EQ(major, 1) << "Binary DMatrix generated by XGBoost: " << Version::String(version) << " is no longer supported. " << "Please process and save your data in current version again."; CHECK(fi->Read(&num_row_, sizeof(num_row_)) == sizeof(num_row_)) << "MetaInfo: invalid format"; From 4076d89f01cb5cd2dcb6be73830d4d49a81afa39 Mon Sep 17 00:00:00 2001 From: fis Date: Tue, 15 Oct 2019 00:18:08 -0400 Subject: [PATCH 09/12] Add version string prefix. --- src/common/version.cc | 17 +++++++++++++++-- src/data/data.cc | 3 ++- tests/cpp/data/test_metainfo.cc | 4 ++-- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/common/version.cc b/src/common/version.cc index a06e18f2fd99..3863aad3890d 100644 --- a/src/common/version.cc +++ b/src/common/version.cc @@ -36,11 +36,22 @@ Version::TripletT Version::Load(Json const& in, bool check) { Version::TripletT Version::Load(dmlc::Stream* fi) { XGBoostVersionT major{0}, minor{0}, patch{0}; // This is only used in DMatrix serialization, so doesn't break model compability. - std::string msg { "Invalid version Format. Are you loading from binary file " - "that's generated by XGBoost < 1.0.0?" }; + std::string msg { "Incorrect version format found in binary file. " + "Binary file from XGBoost < 1.0.0 is no longer supported. " + "Please generate it again." }; + std::string verstr { "version:" }, read; + read.resize(verstr.size(), 0); + + CHECK_EQ(fi->Read(&read[0], verstr.size()), verstr.size()) << msg; + if (verstr != read) { + // read might contain `\0` that terminates the string. + LOG(FATAL) << msg; + } + CHECK_EQ(fi->Read(&major, sizeof(major)), sizeof(major)) << msg; CHECK_EQ(fi->Read(&minor, sizeof(major)), sizeof(minor)) << msg; CHECK_EQ(fi->Read(&patch, sizeof(major)), sizeof(patch)) << msg; + return std::make_tuple(major, minor, patch); } @@ -55,6 +66,8 @@ void Version::Save(Json* out) { void Version::Save(dmlc::Stream* fo) { XGBoostVersionT major, minor, patch; std::tie(major, minor, patch) = Self(); + std::string verstr { "version:" }; + fo->Write(&verstr[0], verstr.size()); fo->Write(&major, sizeof(major)); fo->Write(&minor, sizeof(minor)); fo->Write(&patch, sizeof(patch)); diff --git a/src/data/data.cc b/src/data/data.cc index 8cf37bff6e18..af68ebcd22f4 100644 --- a/src/data/data.cc +++ b/src/data/data.cc @@ -54,7 +54,8 @@ void MetaInfo::LoadBinary(dmlc::Stream *fi) { // version of DMatrix. CHECK_EQ(major, 1) << "Binary DMatrix generated by XGBoost: " << Version::String(version) << " is no longer supported. " - << "Please process and save your data in current version again."; + << "Please process and save your data in current version: " + << Version::String(Version::Self()) << " again."; CHECK(fi->Read(&num_row_, sizeof(num_row_)) == sizeof(num_row_)) << "MetaInfo: invalid format"; CHECK(fi->Read(&num_col_, sizeof(num_col_)) == sizeof(num_col_)) << "MetaInfo: invalid format"; CHECK(fi->Read(&num_nonzero_, sizeof(num_nonzero_)) == sizeof(num_nonzero_)) diff --git a/tests/cpp/data/test_metainfo.cc b/tests/cpp/data/test_metainfo.cc index d6f14ab2624a..c031dd9ed3e2 100644 --- a/tests/cpp/data/test_metainfo.cc +++ b/tests/cpp/data/test_metainfo.cc @@ -58,8 +58,8 @@ TEST(MetaInfo, SaveLoadBinary) { info.SaveBinary(fs.get()); } - ASSERT_EQ(GetFileSize(tmp_file), 84) - << "Expected saved binary file size to be same as object size"; + ASSERT_EQ(GetFileSize(tmp_file), 92) + << "Expected saved binary file size to be same as object size"; std::unique_ptr fs { dmlc::Stream::Create(tmp_file.c_str(), "r") From 434e33cd3906811a9a7a201da5afbb7ef6c30089 Mon Sep 17 00:00:00 2001 From: fis Date: Wed, 16 Oct 2019 01:44:00 -0400 Subject: [PATCH 10/12] Address reviewer's comments. --- include/xgboost/c_api.h | 3 +++ src/c_api/c_api.cc | 12 +++++++++--- src/common/version.cc | 4 ++-- tests/cpp/c_api/test_c_api.cc | 6 ++++++ 4 files changed, 20 insertions(+), 5 deletions(-) diff --git a/include/xgboost/c_api.h b/include/xgboost/c_api.h index 9466ee207700..2a9d492ac034 100644 --- a/include/xgboost/c_api.h +++ b/include/xgboost/c_api.h @@ -60,6 +60,9 @@ typedef struct { // NOLINT(*) /*! * \brief Return the version of the XGBoost library being currently used. + * + * The output variable is only written if it's not NULL. + * * \param major Store the major version number * \param minor Store the minor version number * \param patch Store the patch (revision) number diff --git a/src/c_api/c_api.cc b/src/c_api/c_api.cc index a8370b045be6..c6369f9f009b 100644 --- a/src/c_api/c_api.cc +++ b/src/c_api/c_api.cc @@ -150,9 +150,15 @@ struct XGBAPIThreadLocalEntry { }; XGB_DLL void XGBoostVersion(int* major, int* minor, int* patch) { - *major = XGBOOST_VER_MAJOR; - *minor = XGBOOST_VER_MINOR; - *patch = XGBOOST_VER_PATCH; + if (major) { + *major = XGBOOST_VER_MAJOR; + } + if (minor) { + *minor = XGBOOST_VER_MINOR; + } + if (patch) { + *patch = XGBOOST_VER_PATCH; + } } // define the threadlocal store. diff --git a/src/common/version.cc b/src/common/version.cc index 3863aad3890d..5f25d105737d 100644 --- a/src/common/version.cc +++ b/src/common/version.cc @@ -39,7 +39,7 @@ Version::TripletT Version::Load(dmlc::Stream* fi) { std::string msg { "Incorrect version format found in binary file. " "Binary file from XGBoost < 1.0.0 is no longer supported. " "Please generate it again." }; - std::string verstr { "version:" }, read; + std::string verstr { u8"version:" }, read; read.resize(verstr.size(), 0); CHECK_EQ(fi->Read(&read[0], verstr.size()), verstr.size()) << msg; @@ -66,7 +66,7 @@ void Version::Save(Json* out) { void Version::Save(dmlc::Stream* fo) { XGBoostVersionT major, minor, patch; std::tie(major, minor, patch) = Self(); - std::string verstr { "version:" }; + std::string verstr { u8"version:" }; fo->Write(&verstr[0], verstr.size()); fo->Write(&major, sizeof(major)); fo->Write(&minor, sizeof(minor)); diff --git a/tests/cpp/c_api/test_c_api.cc b/tests/cpp/c_api/test_c_api.cc index d61d1dcedd1d..e0dce427470c 100644 --- a/tests/cpp/c_api/test_c_api.cc +++ b/tests/cpp/c_api/test_c_api.cc @@ -63,3 +63,9 @@ TEST(c_api, XGDMatrixCreateFromMat_omp) { delete dmat; } } + +TEST(c_api, Version) { + int patch {0}; + XGBoostVersion(NULL, NULL, &patch); // make sure it doesn't crash + ASSERT_EQ(patch, XGBOOST_VER_PATCH); +} From cc6952c977f6aa36e21d617aad5ec00d58423004 Mon Sep 17 00:00:00 2001 From: fis Date: Wed, 16 Oct 2019 03:11:25 -0400 Subject: [PATCH 11/12] Lint. --- tests/cpp/c_api/test_c_api.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/cpp/c_api/test_c_api.cc b/tests/cpp/c_api/test_c_api.cc index e0dce427470c..256068d551e7 100644 --- a/tests/cpp/c_api/test_c_api.cc +++ b/tests/cpp/c_api/test_c_api.cc @@ -66,6 +66,6 @@ TEST(c_api, XGDMatrixCreateFromMat_omp) { TEST(c_api, Version) { int patch {0}; - XGBoostVersion(NULL, NULL, &patch); // make sure it doesn't crash + XGBoostVersion(NULL, NULL, &patch); // NOLINT ASSERT_EQ(patch, XGBOOST_VER_PATCH); } From 59caa8a186666bb5f6d0469763eaf207d47d5ef9 Mon Sep 17 00:00:00 2001 From: Hyunsu Philip Cho Date: Tue, 22 Oct 2019 16:40:06 +0000 Subject: [PATCH 12/12] Simplify prefetch detection in CMakeLists.txt --- CMakeLists.txt | 2 ++ cmake/FindPrefetchIntrinsics.cmake | 22 ++++++++++++++++++++++ cmake/build_config.h.in | 15 ++------------- src/CMakeLists.txt | 29 ----------------------------- 4 files changed, 26 insertions(+), 42 deletions(-) create mode 100644 cmake/FindPrefetchIntrinsics.cmake diff --git a/CMakeLists.txt b/CMakeLists.txt index 4fd8c7159fa1..071878b7f6b5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -13,6 +13,8 @@ if (CMAKE_COMPILER_IS_GNUCC AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 5.0) message(FATAL_ERROR "GCC version must be at least 5.0!") endif() +include(${xgboost_SOURCE_DIR}/cmake/FindPrefetchIntrinsics.cmake) +find_prefetch_intrinsics() include(${xgboost_SOURCE_DIR}/cmake/Version.cmake) write_version() set_default_configuration_release() diff --git a/cmake/FindPrefetchIntrinsics.cmake b/cmake/FindPrefetchIntrinsics.cmake new file mode 100644 index 000000000000..b00ff57d717f --- /dev/null +++ b/cmake/FindPrefetchIntrinsics.cmake @@ -0,0 +1,22 @@ +function (find_prefetch_intrinsics) + include(CheckCXXSourceCompiles) + check_cxx_source_compiles(" + #include + int main() { + char data = 0; + const char* address = &data; + _mm_prefetch(address, _MM_HINT_NTA); + return 0; + } + " XGBOOST_MM_PREFETCH_PRESENT) + check_cxx_source_compiles(" + int main() { + char data = 0; + const char* address = &data; + __builtin_prefetch(address, 0, 0); + return 0; + } + " XGBOOST_BUILTIN_PREFETCH_PRESENT) + set(XGBOOST_MM_PREFETCH_PRESENT ${XGBOOST_MM_PREFETCH_PRESENT} PARENT_SCOPE) + set(XGBOOST_BUILTIN_PREFETCH_PRESENT ${XGBOOST_BUILTIN_PREFETCH_PRESENT} PARENT_SCOPE) +endfunction (find_prefetch_intrinsics) diff --git a/cmake/build_config.h.in b/cmake/build_config.h.in index 3bfab88dc59b..74d980cdfe95 100644 --- a/cmake/build_config.h.in +++ b/cmake/build_config.h.in @@ -7,19 +7,8 @@ #ifndef XGBOOST_BUILD_CONFIG_H_ #define XGBOOST_BUILD_CONFIG_H_ -// These check are for Makefile. -#if !defined(XGBOOST_MM_PREFETCH_PRESENT) && !defined(XGBOOST_BUILTIN_PREFETCH_PRESENT) -/* default logic for software pre-fetching */ -#if (defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_AMD64))) || defined(__INTEL_COMPILER) -// Enable _mm_prefetch for Intel compiler and MSVC+x86 - #define XGBOOST_MM_PREFETCH_PRESENT - #define XGBOOST_BUILTIN_PREFETCH_PRESENT -#elif defined(__GNUC__) -// Enable __builtin_prefetch for GCC -#define XGBOOST_BUILTIN_PREFETCH_PRESENT -#endif // GUARDS - -#endif // !defined(XGBOOST_MM_PREFETCH_PRESENT) && !defined() +#cmakedefine XGBOOST_MM_PREFETCH_PRESENT +#cmakedefine XGBOOST_BUILTIN_PREFETCH_PRESENT #define XGBOOST_VER_MAJOR @xgboost_VERSION_MAJOR@ #define XGBOOST_VER_MINOR @xgboost_VERSION_MINOR@ diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index f2b29897b786..ad3af95564fc 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -1,25 +1,6 @@ file(GLOB_RECURSE CPU_SOURCES *.cc *.h) list(REMOVE_ITEM CPU_SOURCES ${PROJECT_SOURCE_DIR}/src/cli_main.cc) -include(CheckCXXSourceCompiles) -check_cxx_source_compiles(" -#include -int main() { - char data = 0; - const char* address = &data; - _mm_prefetch(address, _MM_HINT_NTA); - return 0; -} -" XGBOOST_MM_PREFETCH_PRESENT) -check_cxx_source_compiles(" -int main() { - char data = 0; - const char* address = &data; - __builtin_prefetch(address, 0, 0); - return 0; -} -" XGBOOST_BUILTIN_PREFETCH_PRESENT) - #-- Object library # Object library is necessary for jvm-package, which creates its own shared # library. @@ -82,16 +63,6 @@ target_compile_definitions(objxgboost -DDMLC_LOG_CUSTOMIZE=1 # enable custom logging $<$>:_MWAITXINTRIN_H_INCLUDED> ${XGBOOST_DEFINITIONS}) -if (XGBOOST_MM_PREFETCH_PRESENT) - target_compile_definitions(objxgboost - PRIVATE - -DXGBOOST_MM_PREFETCH_PRESENT=1) -endif(XGBOOST_MM_PREFETCH_PRESENT) -if (XGBOOST_BUILTIN_PREFETCH_PRESENT) - target_compile_definitions(objxgboost - PRIVATE - -DXGBOOST_BUILTIN_PREFETCH_PRESENT=1) -endif (XGBOOST_BUILTIN_PREFETCH_PRESENT) if (USE_OPENMP) find_package(OpenMP REQUIRED)