diff --git a/src/rinterface.h b/src/rinterface.h index 02e016e0c17..4e1c9f5f00a 100644 --- a/src/rinterface.h +++ b/src/rinterface.h @@ -27,6 +27,7 @@ SEXP R_igraph_add_env(SEXP graph); +void R_igraph_attribute_clean_preserve_list(); void R_igraph_set_in_r_check(bool set); void R_igraph_error(void); void R_igraph_warning(void); @@ -34,6 +35,7 @@ void R_igraph_interrupt(void); #define IGRAPH_R_CHECK(func) \ do { \ + R_igraph_attribute_clean_preserve_list(); \ R_igraph_set_in_r_check(true); \ igraph_error_type_t __c = func; \ R_igraph_set_in_r_check(false); \ diff --git a/src/rinterface_extra.c b/src/rinterface_extra.c index b8840bb379d..9535ae2a9c2 100644 --- a/src/rinterface_extra.c +++ b/src/rinterface_extra.c @@ -340,13 +340,32 @@ int R_SEXP_to_attr_comb(SEXP input, igraph_attribute_combination_t *comb) { return 0; } -static SEXP R_igraph_attribute_protected=0; -static long int R_igraph_attribute_protected_size=0; +static SEXP R_igraph_attribute_preserve_list; -void R_igraph_attribute_protected_destroy(void *dummy) { - R_ReleaseObject(R_igraph_attribute_protected); - R_igraph_attribute_protected=0; - R_igraph_attribute_protected_size=0; +void R_igraph_attribute_add_to_preserve_list(SEXP attr) { + if (!R_igraph_attribute_preserve_list) { + // We don't care about freeing this, typically this is just a single node + R_igraph_attribute_preserve_list = Rf_cons(R_NilValue, R_NilValue); + R_PreserveObject(R_igraph_attribute_preserve_list); + } + + // Create a new node, add it to the head of the list. + SEXP node = Rf_cons(attr, CDR(R_igraph_attribute_preserve_list)); + SETCDR(R_igraph_attribute_preserve_list, node); +} + +void R_igraph_attribute_clean_preserve_list() { + if (R_igraph_attribute_preserve_list) { + // Mark the entire list available for garbage collection. + // Attributes that have been assigned to an R graph object will remain protected. + // Dangling attributes will be GC-ed eventually. + + // This is called *before* entering an igraph function that might allocate + // attributes; after such a function returns, we need to keep preserving + // all attributes because they may be put into R graph objects + // and returned to R. + SETCDR(R_igraph_attribute_preserve_list, R_NilValue); + } } int R_igraph_attribute_init(igraph_t *graph, igraph_vector_ptr_t *attr) { @@ -355,35 +374,10 @@ int R_igraph_attribute_init(igraph_t *graph, igraph_vector_ptr_t *attr) { long int attrno; int px = 0; - if (R_igraph_attribute_protected) { - long int ll=GET_LENGTH(R_igraph_attribute_protected); - if (ll == R_igraph_attribute_protected_size) { - SEXP newl = NEW_LIST(2*ll); - long int i; - PROTECT(newl); px++; - for (i=0; iattr; - REAL(VECTOR_ELT(attr, 0))[1] -= 1; /* refcount for igraph_t */ - if (!R_igraph_attribute_protected && - REAL(VECTOR_ELT(attr, 0))[1]==0 && - REAL(VECTOR_ELT(attr, 0))[2]==1) { - R_ReleaseObject(attr); - } + // Owned by the R graph object, will be garbage-collected graph->attr=0; } @@ -464,11 +452,6 @@ int R_igraph_attribute_copy(igraph_t *to, const igraph_t *from, SEXP fromattr=from->attr; if (ga && va && ea) { to->attr=from->attr; - REAL(VECTOR_ELT(fromattr, 0))[1] += 1; /* refcount only */ - if (!R_igraph_attribute_protected && - REAL(VECTOR_ELT(fromattr, 0))[1] == 1) { - R_PreserveObject(to->attr); - } } else { R_igraph_attribute_init(to,0); /* Sets up many things */ SEXP toattr=to->attr; @@ -587,39 +570,7 @@ void R_igraph_attribute_add_vertices_append(SEXP val, long int nv, SEXP R_igraph_attribute_add_vertices_dup(SEXP attr) { SEXP newattr=Rf_duplicate(attr); - int px = 0; - - if (R_igraph_attribute_protected) { - PROTECT(newattr); px++; - } else { - R_PreserveObject(newattr); - } - - REAL(VECTOR_ELT(attr, 0))[1] -= 1; - if (!R_igraph_attribute_protected && - REAL(VECTOR_ELT(attr, 0))[1] == 0) { - R_ReleaseObject(attr); - } - REAL(VECTOR_ELT(newattr, 0))[0] = 0; - REAL(VECTOR_ELT(newattr, 0))[1] = 1; - if (R_igraph_attribute_protected) { - long int pos, alen=LENGTH(VECTOR_ELT(attr, 0)); - if (alen == 4) { - pos=REAL(VECTOR_ELT(attr, 0))[3]; - SET_VECTOR_ELT(R_igraph_attribute_protected, pos, newattr); - } else { - SEXP tmp=PROTECT(NEW_NUMERIC(4)); px++; - REAL(tmp)[0] = REAL(VECTOR_ELT(attr, 0))[0]; - REAL(tmp)[1] = REAL(VECTOR_ELT(attr, 0))[1]; - REAL(tmp)[2] = REAL(VECTOR_ELT(attr, 0))[2]; - pos = REAL(tmp)[3] = R_igraph_attribute_protected_size; - R_igraph_attribute_protected_size += 1; - SET_VECTOR_ELT(newattr, 0, tmp); - } - SET_VECTOR_ELT(R_igraph_attribute_protected, pos, newattr); - } - - UNPROTECT(px); + R_igraph_attribute_add_to_preserve_list(newattr); return newattr; } @@ -631,10 +582,8 @@ int R_igraph_attribute_add_vertices(igraph_t *graph, long int nv, long int valno, i, origlen, nattrno, newattrs; int px = 0; - if (REAL(VECTOR_ELT(attr, 0))[0]+REAL(VECTOR_ELT(attr, 0))[1] > 1) { - SEXP newattr = PROTECT(R_igraph_attribute_add_vertices_dup(attr)); px++; - attr=graph->attr=newattr; - } + SEXP newattr = PROTECT(R_igraph_attribute_add_vertices_dup(attr)); px++; + attr=graph->attr=newattr; val=VECTOR_ELT(attr, 2); valno=GET_LENGTH(val); @@ -702,17 +651,9 @@ int R_igraph_attribute_add_vertices(igraph_t *graph, long int nv, /* SEXP attr=graph->attr; */ /* SEXP eal, val; */ /* long int valno, ealno, i; */ -/* if (REAL(VECTOR_ELT(attr, 0))[0]+REAL(VECTOR_ELT(attr, 0))[1] > 1) { */ -/* SEXP newattr; */ -/* PROTECT(newattr=Rf_duplicate(attr)); */ -/* REAL(VECTOR_ELT(attr, 0))[1] -= 1; */ -/* if (REAL(VECTOR_ELT(attr, 0))[1] == 0) { */ -/* R_ReleaseObject(attr); */ -/* } */ -/* REAL(VECTOR_ELT(newattr, 0))[0] = 0; */ -/* REAL(VECTOR_ELT(newattr, 0))[1] = 1; */ -/* attr=graph->attr=newattr; */ -/* } */ +/* SEXP newattr; */ +/* PROTECT(newattr=Rf_duplicate(attr)); */ +/* attr=graph->attr=newattr; */ /* /\* Vertices *\/ */ /* val=VECTOR_ELT(attr, 2); */ @@ -772,39 +713,9 @@ int R_igraph_attribute_permute_vertices_same(const igraph_t *graph, SEXP ss; int px = 0; - /* We copy if we need to */ - if (REAL(VECTOR_ELT(attr, 0))[0]+REAL(VECTOR_ELT(attr, 0))[1] > 1) { - SEXP newattr = Rf_duplicate(attr); - if (R_igraph_attribute_protected) { - PROTECT(newattr); px++; - } else { - R_PreserveObject(newattr); - } - REAL(VECTOR_ELT(attr, 0))[1] -= 1; - if (!R_igraph_attribute_protected && - REAL(VECTOR_ELT(attr, 0))[1] == 0) { - R_ReleaseObject(attr); - } - REAL(VECTOR_ELT(newattr, 0))[0] = 0; - REAL(VECTOR_ELT(newattr, 0))[1] = 1; - if (R_igraph_attribute_protected) { - long int pos, alen=LENGTH(VECTOR_ELT(attr, 0)); - if (alen == 4) { - pos=REAL(VECTOR_ELT(attr, 0))[3]; - SET_VECTOR_ELT(R_igraph_attribute_protected, pos, newattr); - } else { - SEXP tmp=PROTECT(NEW_NUMERIC(4)); px++; - REAL(tmp)[0] = REAL(VECTOR_ELT(attr, 0))[0]; - REAL(tmp)[1] = REAL(VECTOR_ELT(attr, 0))[1]; - REAL(tmp)[2] = REAL(VECTOR_ELT(attr, 0))[2]; - pos = REAL(tmp)[3] = R_igraph_attribute_protected_size; - R_igraph_attribute_protected_size += 1; - SET_VECTOR_ELT(newattr, 0, tmp); - } - SET_VECTOR_ELT(R_igraph_attribute_protected, pos, newattr); - } - attr=newgraph->attr=newattr; - } + SEXP newattr = Rf_duplicate(attr); + R_igraph_attribute_add_to_preserve_list(newattr); + attr=newgraph->attr=newattr; val=VECTOR_ELT(attr,2); valno=GET_LENGTH(val); @@ -890,39 +801,7 @@ int R_igraph_attribute_permute_vertices(const igraph_t *graph, SEXP R_igraph_attribute_add_edges_dup(SEXP attr) { SEXP newattr=Rf_duplicate(attr); - int px = 0; - - if (R_igraph_attribute_protected) { - PROTECT(newattr); px++; - } else { - R_PreserveObject(newattr); - } - - REAL(VECTOR_ELT(attr, 0))[1] -= 1; - if (!R_igraph_attribute_protected && - REAL(VECTOR_ELT(attr, 0))[1] == 0) { - R_ReleaseObject(attr); - } - REAL(VECTOR_ELT(newattr, 0))[0] = 0; - REAL(VECTOR_ELT(newattr, 0))[1] = 1; - if (R_igraph_attribute_protected) { - long int pos, alen=LENGTH(VECTOR_ELT(attr, 0)); - if (alen == 4) { - pos=REAL(VECTOR_ELT(attr, 0))[3]; - SET_VECTOR_ELT(R_igraph_attribute_protected, pos, newattr); - } else { - SEXP tmp=PROTECT(NEW_NUMERIC(4)); px++; - REAL(tmp)[0] = REAL(VECTOR_ELT(attr, 0))[0]; - REAL(tmp)[1] = REAL(VECTOR_ELT(attr, 0))[1]; - REAL(tmp)[2] = REAL(VECTOR_ELT(attr, 0))[2]; - pos = REAL(tmp)[3] = R_igraph_attribute_protected_size; - R_igraph_attribute_protected_size += 1; - SET_VECTOR_ELT(newattr, 0, tmp); - } - SET_VECTOR_ELT(R_igraph_attribute_protected, pos, newattr); - } - - UNPROTECT(px); + R_igraph_attribute_add_to_preserve_list(newattr); return newattr; } @@ -1041,10 +920,8 @@ int R_igraph_attribute_add_edges(igraph_t *graph, if (igraph_vector_init(&news, 0)) Rf_error("Out of memory"); IGRAPH_FINALLY(igraph_vector_destroy, &news); - if (REAL(VECTOR_ELT(attr, 0))[0] + REAL(VECTOR_ELT(attr, 0))[1] > 1) { - SEXP newattr = PROTECT(R_igraph_attribute_add_edges_dup(attr)); px++; - attr=graph->attr=newattr; - } + SEXP newattr = PROTECT(R_igraph_attribute_add_edges_dup(attr)); px++; + attr=graph->attr=newattr; eal=VECTOR_ELT(attr, 3); ealno=GET_LENGTH(eal); @@ -1109,17 +986,9 @@ int R_igraph_attribute_add_edges(igraph_t *graph, /* SEXP attr=graph->attr; */ /* SEXP eal; */ /* long int ealno, i; */ -/* if (REAL(VECTOR_ELT(attr, 0))[0]+REAL(VECTOR_ELT(attr, 0))[1] > 1) { */ -/* SEXP newattr; */ -/* PROTECT(newattr=Rf_duplicate(attr)); */ -/* REAL(VECTOR_ELT(attr, 0))[1] -= 1; */ -/* if (REAL(VECTOR_ELT(attr, 0))[1] == 0) { */ -/* R_ReleaseObject(attr); */ -/* } */ -/* REAL(VECTOR_ELT(newattr, 0))[0] = 0; */ -/* REAL(VECTOR_ELT(newattr, 0))[1] = 1; */ -/* attr=graph->attr=newattr; */ -/* } */ +/* SEXP newattr; */ +/* PROTECT(newattr=Rf_duplicate(attr)); */ +/* attr=graph->attr=newattr; */ /* eal=VECTOR_ELT(attr, 3); */ /* ealno=GET_LENGTH(eal); */ @@ -1156,39 +1025,9 @@ int R_igraph_attribute_permute_edges_same(const igraph_t *graph, SEXP ss; int px = 0; - /* We copy if we need to */ - if (REAL(VECTOR_ELT(attr, 0))[0]+REAL(VECTOR_ELT(attr, 0))[1] > 1) { - SEXP newattr=Rf_duplicate(attr); - if (R_igraph_attribute_protected) { - PROTECT(newattr); px++; - } else { - R_PreserveObject(newattr); - } - REAL(VECTOR_ELT(attr, 0))[1] -= 1; - if (!R_igraph_attribute_protected && - REAL(VECTOR_ELT(attr, 0))[1] == 0) { - R_ReleaseObject(attr); - } - REAL(VECTOR_ELT(newattr, 0))[0] = 0; - REAL(VECTOR_ELT(newattr, 0))[1] = 1; - if (R_igraph_attribute_protected) { - long int pos, alen=LENGTH(VECTOR_ELT(attr, 0)); - if (alen == 4) { - pos=REAL(VECTOR_ELT(attr, 0))[3]; - SET_VECTOR_ELT(R_igraph_attribute_protected, pos, newattr); - } else { - SEXP tmp=PROTECT(NEW_NUMERIC(4)); px++; - REAL(tmp)[0] = REAL(VECTOR_ELT(attr, 0))[0]; - REAL(tmp)[1] = REAL(VECTOR_ELT(attr, 0))[1]; - REAL(tmp)[2] = REAL(VECTOR_ELT(attr, 0))[2]; - pos = REAL(tmp)[3] = R_igraph_attribute_protected_size; - R_igraph_attribute_protected_size += 1; - SET_VECTOR_ELT(newattr, 0, tmp); - } - SET_VECTOR_ELT(R_igraph_attribute_protected, pos, newattr); - } - attr=newgraph->attr=newattr; - } + SEXP newattr=Rf_duplicate(attr); + R_igraph_attribute_add_to_preserve_list(newattr); + attr=newgraph->attr=newattr; eal=VECTOR_ELT(attr,3); ealno=GET_LENGTH(eal); @@ -2073,10 +1912,6 @@ int R_igraph_attribute_combine_vertices(const igraph_t *graph, } } - /* Not safe to UNPROTECT attributes */ - REAL(VECTOR_ELT(attr, 0))[2]=0; - REAL(VECTOR_ELT(toattr, 0))[2]=0; - PROTECT(res=NEW_LIST(keepno)); px++; PROTECT(newnames=NEW_CHARACTER(keepno)); px++; for (i=0, j=0; iattr); - REAL(VECTOR_ELT(graph->attr, 0))[0] += 1; /* Environment for vertex/edge seqs */ SET_VECTOR_ELT(result, igraph_t_idx_env, R_NilValue); @@ -3718,8 +3540,6 @@ int R_SEXP_to_igraph(SEXP graph, igraph_t *res) { R_igraph_get_is(graph, &res->is); /* attributes */ - REAL(VECTOR_ELT(VECTOR_ELT(graph, igraph_t_idx_attr), 0))[0] = 1; /* R objects refcount */ - REAL(VECTOR_ELT(VECTOR_ELT(graph, igraph_t_idx_attr), 0))[1] = 0; /* igraph_t objects */ res->attr=VECTOR_ELT(graph, igraph_t_idx_attr); return 0; @@ -3750,9 +3570,7 @@ int R_SEXP_to_igraph_copy(SEXP graph, igraph_t *res) { igraph_vector_copy(&res->is, &is); /* attributes */ - REAL(VECTOR_ELT(VECTOR_ELT(graph, igraph_t_idx_attr), 0))[0] = 1; /* R objects */ - REAL(VECTOR_ELT(VECTOR_ELT(graph, igraph_t_idx_attr), 0))[1] = 1; /* igraph_t objects */ - R_PreserveObject(res->attr=VECTOR_ELT(graph, igraph_t_idx_attr)); + res->attr=VECTOR_ELT(graph, igraph_t_idx_attr); return 0; } @@ -5814,10 +5632,6 @@ SEXP R_igraph_decompose(SEXP graph, SEXP pmode, SEXP pmaxcompno, SEXP result; long int i; - R_PreserveObject(R_igraph_attribute_protected=NEW_LIST(100)); - R_igraph_attribute_protected_size=0; - IGRAPH_FINALLY(R_igraph_attribute_protected_destroy, 0); - R_SEXP_to_igraph(graph, &g); igraph_vector_ptr_init(&comps, 0); IGRAPH_FINALLY(igraph_vector_ptr_destroy, &comps); @@ -5831,8 +5645,7 @@ SEXP R_igraph_decompose(SEXP graph, SEXP pmode, SEXP pmaxcompno, igraph_vector_ptr_destroy(&comps); UNPROTECT(1); - IGRAPH_FINALLY_CLEAN(2); - R_igraph_attribute_protected_destroy(0); + IGRAPH_FINALLY_CLEAN(1); return result; } @@ -7143,10 +6956,6 @@ SEXP R_igraph_neighborhood_graphs(SEXP graph, SEXP pvids, SEXP porder, igraph_integer_t mindist=INTEGER(pmindist)[0]; SEXP result; - R_PreserveObject(R_igraph_attribute_protected=NEW_LIST(100)); - R_igraph_attribute_protected_size=0; - IGRAPH_FINALLY(R_igraph_attribute_protected_destroy, 0); - R_SEXP_to_igraph(graph, &g); R_SEXP_to_igraph_vs(pvids, &g, &vids); igraph_vector_ptr_init(&res, 0); @@ -7162,8 +6971,6 @@ SEXP R_igraph_neighborhood_graphs(SEXP graph, SEXP pvids, SEXP porder, igraph_vs_destroy(&vids); UNPROTECT(1); - IGRAPH_FINALLY_CLEAN(1); - R_igraph_attribute_protected_destroy(0); return result; } @@ -9283,10 +9090,6 @@ SEXP R_igraph_graphlets(SEXP graph, SEXP weights, SEXP niter) { SEXP Mu; SEXP result, names; - R_PreserveObject(R_igraph_attribute_protected=NEW_LIST(100)); - R_igraph_attribute_protected_size=0; - IGRAPH_FINALLY(R_igraph_attribute_protected_destroy, 0); - /* Convert input */ R_SEXP_to_igraph(graph, &c_graph); if (!Rf_isNull(weights)) { R_SEXP_to_vector(weights, &c_weights); } @@ -9318,8 +9121,6 @@ SEXP R_igraph_graphlets(SEXP graph, SEXP weights, SEXP niter) { SET_NAMES(result, names); UNPROTECT(4); - IGRAPH_FINALLY_CLEAN(1); - R_igraph_attribute_protected_destroy(0); return(result); } @@ -9338,10 +9139,6 @@ SEXP R_igraph_graphlets_candidate_basis(SEXP graph, SEXP weights) { SEXP result, names; - R_PreserveObject(R_igraph_attribute_protected=NEW_LIST(100)); - R_igraph_attribute_protected_size=0; - IGRAPH_FINALLY(R_igraph_attribute_protected_destroy, 0); - /* Convert input */ R_SEXP_to_igraph(graph, &c_graph); if (!Rf_isNull(weights)) { R_SEXP_to_vector(weights, &c_weights); } @@ -9372,8 +9169,6 @@ SEXP R_igraph_graphlets_candidate_basis(SEXP graph, SEXP weights) { SET_NAMES(result, names); UNPROTECT(4); - IGRAPH_FINALLY_CLEAN(1); - R_igraph_attribute_protected_destroy(0); return(result); } diff --git a/tests/testthat/test-attributes.R b/tests/testthat/test-attributes.R index 187cf6852dd..2aa2a26c998 100644 --- a/tests/testthat/test-attributes.R +++ b/tests/testthat/test-attributes.R @@ -254,3 +254,93 @@ test_that("setting NULL attributes works and doesn't change the input (#466)", { expect_identical(set_edge_attr(g, "foo", value = NULL), g) expect_identical(set_edge_attr(g, "foo", 1:3, value = NULL), g) }) + +test_that("GRAPH attributes are destroyed when the graph is destroyed", { + finalized <- FALSE + finalizer <- function(e) { + finalized <<- TRUE + } + + env <- new.env(parent = emptyenv()) + reg.finalizer(env, finalizer) + + g <- make_ring(1) + g$a <- list(env) + rm(env) + gc() + expect_false(finalized) + + rm(g) + gc() + expect_true(finalized) +}) + +test_that("vertex attributes are destroyed when the graph is destroyed", { + finalized <- FALSE + finalizer <- function(e) { + finalized <<- TRUE + } + + env <- new.env(parent = emptyenv()) + reg.finalizer(env, finalizer) + + g <- make_ring(1) + V(g)$a <- list(env) + rm(env) + gc() + expect_false(finalized) + + g <- add_vertices(g, 1) + gc() + expect_false(finalized) + + g <- delete_vertices(g, 2) + gc() + expect_false(finalized) + + # Called for the side effect of clearing the protect list + make_empty_graph() + expect_false(finalized) + + rm(g) + + gc() + expect_true(finalized) +}) + +test_that("edge attributes are destroyed when the graph is destroyed", { + finalized <- FALSE + finalizer <- function(e) { + finalized <<- TRUE + } + + env <- new.env(parent = emptyenv()) + reg.finalizer(env, finalizer) + + g <- make_ring(2) + E(g)$a <- list(env) + rm(env) + gc() + expect_false(finalized) + + g <- add_vertices(g, 1) + gc() + expect_false(finalized) + + g <- add_edges(g, c(2, 3)) + gc() + expect_false(finalized) + + g <- delete_edges(g, 2) + gc() + expect_false(finalized) + + # Called for the side effect of clearing the protect list + make_empty_graph() + expect_false(finalized) + + rm(g) + + gc() + expect_true(finalized) +}) diff --git a/tests/testthat/test-decompose.graph.R b/tests/testthat/test-decompose.graph.R index da4321e03dc..a4a8bd82079 100644 --- a/tests/testthat/test-decompose.graph.R +++ b/tests/testthat/test-decompose.graph.R @@ -35,3 +35,13 @@ test_that("decompose keeps attributes", { expect_that(E(d[[1]])$name, equals(e1)) expect_that(E(d[[2]])$name, equals(e2)) }) + +test_that("decompose protects correctly", { + g <- make_graph(integer(), n = 10001) + V(g)$a <- 1 + + torture <- gctorture2(10001) + on.exit(gctorture2(torture)) + + length(decompose(g)) +})