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.

Sign up for GitHub

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

Jump to bottom

Port R compatibility patches from 1.0.0 release branch #5577

Merged
merged 2 commits into from
Apr 22, 2020
Merged

Port R compatibility patches from 1.0.0 release branch #5577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d681d95
Don't use memset to set struct when compiling for R
hcho3 Mar 22, 2020
5839d39
Support 32-bit Solaris target for R package
hcho3 Mar 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions R-package/tests/testthat/test_helpers.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ require(vcd, quietly = TRUE)

float_tolerance = 5e-6

# disable some tests for Win32
win32_flag = .Platform$OS.type == "windows" && .Machine$sizeof.pointer != 8
# disable some tests for 32-bit environment
flag_32bit = .Machine$sizeof.pointer != 8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong with our 32 bit build?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the unit tests will fail because 32-bit platform uses x87 unit, whereas 64-bit uses SSE. See #3723 for context. In the long run, we should re-design tests so that they work equally well on 32-bit and 64-bit systems.

Support for 32-bit systems is mandated by CRAN submission policy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracking issue: #5578


set.seed(1982)
data(Arthritis)
Expand Down Expand Up @@ -44,7 +44,7 @@ mbst.GLM <- xgboost(data = as.matrix(iris[, -5]), label = mlabel, verbose = 0,


test_that("xgb.dump works", {
if (!win32_flag)
if (!flag_32bit)
expect_length(xgb.dump(bst.Tree), 200)
dump_file = file.path(tempdir(), 'xgb.model.dump')
expect_true(xgb.dump(bst.Tree, dump_file, with_stats = T))
Expand All @@ -54,7 +54,7 @@ test_that("xgb.dump works", {
# JSON format
dmp <- xgb.dump(bst.Tree, dump_format = "json")
expect_length(dmp, 1)
if (!win32_flag)
if (!flag_32bit)
expect_length(grep('nodeid', strsplit(dmp, '\n')[[1]]), 188)
})

Expand Down Expand Up @@ -256,7 +256,7 @@ test_that("xgb.model.dt.tree works with and without feature names", {
names.dt.trees <- c("Tree", "Node", "ID", "Feature", "Split", "Yes", "No", "Missing", "Quality", "Cover")
dt.tree <- xgb.model.dt.tree(feature_names = feature.names, model = bst.Tree)
expect_equal(names.dt.trees, names(dt.tree))
if (!win32_flag)
if (!flag_32bit)
expect_equal(dim(dt.tree), c(188, 10))
expect_output(str(dt.tree), 'Feature.*\\"Age\\"')

Expand All @@ -283,7 +283,7 @@ test_that("xgb.model.dt.tree throws error for gblinear", {

test_that("xgb.importance works with and without feature names", {
importance.Tree <- xgb.importance(feature_names = feature.names, model = bst.Tree)
if (!win32_flag)
if (!flag_32bit)
expect_equal(dim(importance.Tree), c(7, 4))
expect_equal(colnames(importance.Tree), c("Feature", "Gain", "Cover", "Frequency"))
expect_output(str(importance.Tree), 'Feature.*\\"Age\\"')
Expand Down
4 changes: 4 additions & 0 deletions src/common/hist_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -831,7 +831,11 @@ void GHistIndexBlockMatrix::Init(const GHistIndexMatrix& gmat,
* \brief fill a histogram by zeros in range [begin, end)
*/
void InitilizeHistByZeroes(GHistRow hist, size_t begin, size_t end) {
#if defined(XGBOOST_STRICT_R_MODE) && XGBOOST_STRICT_R_MODE == 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you run a small benchmark on this? You can call std::fill(hist.data() + begin, hist.data() + end, tree::GradStats() to avoid overhead created by span. If the perf gain by memset is negligible, we can mainline it to master.

Copy link
Contributor

@SmirnovEgorRu SmirnovEgorRu Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use in both cases:

using FPType = decltype(tree::GradStats::sum_grad);
FPType* pdata = reinterpret_cast<FPType*>(hist.data());
std::fill_n(pdata + 2 * begin, 2 * (end-begin), 0);

It should have the same perf as memset

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's come back to this after 1.1.0 release.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracking issue: #5579

std::fill(hist.begin() + begin, hist.begin() + end, tree::GradStats());
#else // defined(XGBOOST_STRICT_R_MODE) && XGBOOST_STRICT_R_MODE == 1
memset(hist.data() + begin, '\0', (end-begin)*sizeof(tree::GradStats));
#endif // defined(XGBOOST_STRICT_R_MODE) && XGBOOST_STRICT_R_MODE == 1
}

/*!
Expand Down
4 changes: 3 additions & 1 deletion src/common/io.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,16 @@ std::string LoadSequentialFile(std::string fname) {
size_t f_size_bytes = fs.st_size;
buffer.resize(f_size_bytes + 1);
int32_t fd = open(fname.c_str(), O_RDONLY);
#if defined(__linux__)
posix_fadvise(fd, 0, 0, POSIX_FADV_SEQUENTIAL);
#endif // defined(__linux__)
ssize_t bytes_read = read(fd, &buffer[0], f_size_bytes);
if (bytes_read < 0) {
close(fd);
ReadErr();
}
close(fd);
#else
#else // defined(__unix__)
FILE *f = fopen(fname.c_str(), "r");
if (f == NULL) {
std::string msg;
Expand Down
4 changes: 2 additions & 2 deletions src/common/observer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@
#define OBSERVER_PRINT LOG(INFO)
#define OBSERVER_ENDL ""
#define OBSERVER_NEWLINE ""
#else
#else // defined(XGBOOST_STRICT_R_MODE) && XGBOOST_STRICT_R_MODE == 1
trivialfis marked this conversation as resolved.
Show resolved Hide resolved
#define OBSERVER_PRINT std::cout << std::setprecision(17)
#define OBSERVER_ENDL std::endl
#define OBSERVER_NEWLINE "\n"
#endif // defined(XGBOOST_STRICT_R_MODE)
#endif // defined(XGBOOST_STRICT_R_MODE) && XGBOOST_STRICT_R_MODE == 1

namespace xgboost {
/*\brief An observer for logging internal data structures.
Expand Down