Skip to content

Follow up on gc race #2899

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

Merged
mattdowle merged 4 commits into from
May 24, 2018
Merged

Follow up on gc race #2899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7e425e7
between.c
mattdowle May 24, 2018
2496060
fsort.c
mattdowle May 24, 2018
837c901
reorder.c
mattdowle May 24, 2018
5a9324f
news item 2 not 1
mattdowle May 24, 2018
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
33 changes: 20 additions & 13 deletions src/between.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,51 +3,51 @@

static double l=0.0, u=0.0;

Rboolean int_upper_closed(SEXP x, R_len_t i) {
static Rboolean int_upper_closed(SEXP x, R_len_t i) {
return (INTEGER(x)[i] == NA_INTEGER || (double)INTEGER(x)[i] <= u ? NA_LOGICAL : FALSE);
}

Rboolean int_upper_open(SEXP x, R_len_t i) {
static Rboolean int_upper_open(SEXP x, R_len_t i) {
return (INTEGER(x)[i] == NA_INTEGER || (double)INTEGER(x)[i] < u ? NA_LOGICAL : FALSE);
}

Rboolean int_lower_closed(SEXP x, R_len_t i) {
static Rboolean int_lower_closed(SEXP x, R_len_t i) {
return (INTEGER(x)[i] == NA_INTEGER || (double)INTEGER(x)[i] >= l ? NA_LOGICAL : FALSE);
}

Rboolean int_lower_open(SEXP x, R_len_t i) {
static Rboolean int_lower_open(SEXP x, R_len_t i) {
return (INTEGER(x)[i] == NA_INTEGER || (double)INTEGER(x)[i] > l ? NA_LOGICAL : FALSE);
}

Rboolean int_both_closed(SEXP x, R_len_t i) {
static Rboolean int_both_closed(SEXP x, R_len_t i) {
return (INTEGER(x)[i] == NA_INTEGER ? NA_LOGICAL : ((double)INTEGER(x)[i] >= l && (double)INTEGER(x)[i] <= u));
}

Rboolean int_both_open(SEXP x, R_len_t i) {
static Rboolean int_both_open(SEXP x, R_len_t i) {
return (INTEGER(x)[i] == NA_INTEGER ? NA_LOGICAL : ((double)INTEGER(x)[i] > l && (double)INTEGER(x)[i] < u));
}

Rboolean double_upper_closed(SEXP x, R_len_t i) {
static Rboolean double_upper_closed(SEXP x, R_len_t i) {
return (ISNAN(REAL(x)[i]) || REAL(x)[i] <= u ? NA_LOGICAL : FALSE);
}

Rboolean double_upper_open(SEXP x, R_len_t i) {
static Rboolean double_upper_open(SEXP x, R_len_t i) {
return (ISNAN(REAL(x)[i]) || REAL(x)[i] < u ? NA_LOGICAL : FALSE);
}

Rboolean double_lower_closed(SEXP x, R_len_t i) {
static Rboolean double_lower_closed(SEXP x, R_len_t i) {
return (ISNAN(REAL(x)[i]) || REAL(x)[i] >= l ? NA_LOGICAL : FALSE);
}

Rboolean double_lower_open(SEXP x, R_len_t i) {
static Rboolean double_lower_open(SEXP x, R_len_t i) {
return (ISNAN(REAL(x)[i]) || REAL(x)[i] > l ? NA_LOGICAL : FALSE);
}

Rboolean double_both_closed(SEXP x, R_len_t i) {
static Rboolean double_both_closed(SEXP x, R_len_t i) {
return (ISNAN(REAL(x)[i]) ? NA_LOGICAL : (REAL(x)[i] >= l && REAL(x)[i] <= u));
}

Rboolean double_both_open(SEXP x, R_len_t i) {
static Rboolean double_both_open(SEXP x, R_len_t i) {
return (ISNAN(REAL(x)[i]) ? NA_LOGICAL : (REAL(x)[i] > l && REAL(x)[i] < u));
}

Expand All @@ -66,11 +66,18 @@ SEXP between(SEXP x, SEXP lower, SEXP upper, SEXP bounds) {
if (!isLogical(bounds) || LOGICAL(bounds)[0] == NA_LOGICAL)
error("incbounds must be logical TRUE/FALSE.");

int nprotect = 0;
if (ALTREP(x)) { x = PROTECT(duplicate(x)); nprotect++; }
if (ALTREP(lower)) { lower = PROTECT(duplicate(lower)); nprotect++; }
if (ALTREP(upper)) { upper = PROTECT(duplicate(upper)); nprotect++; }
if (ALTREP(bounds)) { bounds = PROTECT(duplicate(bounds)); nprotect++; }

// no support for int64 yet (only handling most common cases)
// coerce to also get NA values properly
lower = PROTECT(coerceVector(lower, REALSXP)); l = REAL(lower)[0];
upper = PROTECT(coerceVector(upper, REALSXP)); u = REAL(upper)[0];
ans = PROTECT(allocVector(LGLSXP, nx));
nprotect += 3;

if (LOGICAL(bounds)[0]) {
fupper = isInteger(x) ? &int_upper_closed : &double_upper_closed;
Expand Down Expand Up @@ -99,6 +106,6 @@ SEXP between(SEXP x, SEXP lower, SEXP upper, SEXP bounds) {
for (i=0; i<nx; i++) LOGICAL(ans)[i] = fboth(x, i);
}
}
UNPROTECT(3);
UNPROTECT(nprotect);
return(ans);
}
6 changes: 4 additions & 2 deletions src/fsort.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ SEXP fsort(SEXP x, SEXP verboseArg) {
// TODO: not only detect if already sorted, but if it is, just return x to save the duplicate

SEXP ansVec = PROTECT(allocVector(REALSXP, xlength(x)));
int nprotect = 1;
double *ans = REAL(ansVec);
// allocate early in case fails if not enough RAM
// TODO: document this is much cheaper than a copy followed by in-place.
Expand All @@ -127,6 +128,8 @@ SEXP fsort(SEXP x, SEXP verboseArg) {
// could be that lastBatchSize == batchSize when i) xlength(x) is multiple of nBatch
// and ii) for small vectors with just one batch

if (ALTREP(x)) { x = PROTECT(duplicate(x)); nprotect++; }

t[1] = wallclock();
double mins[nBatch], maxs[nBatch];
#pragma omp parallel for schedule(dynamic) num_threads(nth)
Expand Down Expand Up @@ -305,8 +308,7 @@ SEXP fsort(SEXP x, SEXP verboseArg) {
Rprintf("%d: %.3f (%4.1f%%)\n", i, t[i]-t[i-1], 100.*(t[i]-t[i-1])/tot);
}

UNPROTECT(1);
UNPROTECT(nprotect);
return(ansVec);
}


6 changes: 5 additions & 1 deletion src/reorder.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,20 @@ SEXP reorder(SEXP x, SEXP order)
error("Column %d is length %d which differs from length of column 1 (%d). Invalid data.table.", i+1, length(v), nrow);
if (SIZEOF(v) > maxSize)
maxSize=SIZEOF(v);
if (ALTREP(v)) SET_VECTOR_ELT(x,i,duplicate(v)); // expand compact vector in place, ready for reordering by reference
}
} else {
if (SIZEOF(x)!=4 && SIZEOF(x)!=8)
error("reorder accepts vectors but this non-VECSXP is type '%s' which isn't yet supported", type2char(TYPEOF(x)));
if (ALTREP(x)) error("Internal error in reorder.c: cannot reorder an ALTREP vector. Please see NEWS item 2 in v1.11.4 and report this as a bug.");
maxSize = SIZEOF(x);
nrow = length(x);
ncol = 1;
}
if (!isInteger(order)) error("order must be an integer vector");
if (length(order) != nrow) error("nrow(x)[%d]!=length(order)[%d]",nrow,length(order));
int nprotect = 0;
if (ALTREP(order)) { order=PROTECT(duplicate(order)); nprotect++; } // TODO: how to fetch range of ALTREP compact vector

R_len_t start = 0;
while (start<nrow && INTEGER(order)[start] == start+1) start++;
Expand Down Expand Up @@ -100,6 +104,7 @@ SEXP reorder(SEXP x, SEXP order)
// size_t, otherwise #5305 (integer overflow in memcpy)
}
for (int i=0; i<nth; i++) free(tmp[i]);
UNPROTECT(nprotect);
return(R_NilValue);
}

Expand Down Expand Up @@ -137,4 +142,3 @@ SEXP setrev(SEXP x) {
return(R_NilValue);
}


2 changes: 1 addition & 1 deletion src/subset.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ SEXP subsetDT(SEXP x, SEXP rows, SEXP cols) {
SETLENGTH(ans, LENGTH(cols));
for (int i=0; i<LENGTH(cols); i++) {
SEXP source, target;
if (ALTREP(VECTOR_ELT(x, INTEGER(cols)[i]-1))) error("Internal error in subset.c: column %d is an ALTREP vector. Please see NEWS item 1 in v1.11.4 and report this as a bug.", INTEGER(cols)[i]);
if (ALTREP(VECTOR_ELT(x, INTEGER(cols)[i]-1))) error("Internal error in subset.c: column %d is an ALTREP vector. Please see NEWS item 2 in v1.11.4 and report this as a bug.", INTEGER(cols)[i]);
target = PROTECT(allocVector(TYPEOF(source=VECTOR_ELT(x, INTEGER(cols)[i]-1)), ansn));
SETLENGTH(target, ansn);
SET_TRUELENGTH(target, ansn);
Expand Down