From ce190c82cdb0eb28faba408e135de212766fdfe5 Mon Sep 17 00:00:00 2001 From: Kevin Ushey Date: Sat, 17 Sep 2022 11:56:45 -0700 Subject: [PATCH 1/7] avoid need for R_NilValue checks in protect code --- NEWS.md | 2 ++ inst/include/cpp11/protect.hpp | 31 ++++++++++++++----------------- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/NEWS.md b/NEWS.md index 2ad5492d..a27c5a58 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,7 @@ # cpp11 (development version) +* Minor performance improvements to the cpp11 protect code. (@kevinushey) + * Modernized the GitHub Actions workflows and updated some internal tests to better align with changes in those workflows and the latest version of R (#279). diff --git a/inst/include/cpp11/protect.hpp b/inst/include/cpp11/protect.hpp index ee332939..1ede5f97 100644 --- a/inst/include/cpp11/protect.hpp +++ b/inst/include/cpp11/protect.hpp @@ -315,16 +315,17 @@ static struct { static SEXP list_ = get_preserve_list(); - // Add a new cell that points to the previous end. - SEXP cell = PROTECT(Rf_cons(list_, CDR(list_))); + // Get referneces to head, tail of the precious list. + SEXP head = list_; + SEXP tail = CDR(list_); + // Add a new cell that points to the previous end. + SEXP cell = PROTECT(Rf_cons(head, tail)); SET_TAG(cell, obj); - SETCDR(list_, cell); - - if (CDR(cell) != R_NilValue) { - SETCAR(CDR(cell), cell); - } + // Update the head + tail to point at this cell. + SETCDR(head, cell); + SETCAR(tail, cell); UNPROTECT(2); @@ -362,19 +363,15 @@ static struct { return; #endif - SEXP before = CAR(token); + SEXP head = CAR(token); + SEXP tail = CDR(token); - SEXP after = CDR(token); - - if (before == R_NilValue && after == R_NilValue) { + if (head == R_NilValue && tail == R_NilValue) { Rf_error("should never happen"); } - SETCDR(before, after); - - if (after != R_NilValue) { - SETCAR(after, before); - } + SETCDR(head, tail); + SETCAR(tail, head); } private: @@ -418,7 +415,7 @@ static struct { if (TYPEOF(preserve_list) != LISTSXP) { preserve_list = get_preserve_xptr_addr(); if (TYPEOF(preserve_list) != LISTSXP) { - preserve_list = Rf_cons(R_NilValue, R_NilValue); + preserve_list = Rf_cons(R_NilValue, Rf_cons(R_NilValue, R_NilValue)); R_PreserveObject(preserve_list); set_preserve_xptr(preserve_list); } From e41d8a56b441742f9006a452ace0127cfb43a176 Mon Sep 17 00:00:00 2001 From: Kevin Ushey Date: Sat, 17 Sep 2022 12:03:11 -0700 Subject: [PATCH 2/7] tweak comments + names --- inst/include/cpp11/protect.hpp | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/inst/include/cpp11/protect.hpp b/inst/include/cpp11/protect.hpp index 1ede5f97..86ac973e 100644 --- a/inst/include/cpp11/protect.hpp +++ b/inst/include/cpp11/protect.hpp @@ -319,11 +319,12 @@ static struct { SEXP head = list_; SEXP tail = CDR(list_); - // Add a new cell that points to the previous end. + // Add a new cell that points to the current head + tail. SEXP cell = PROTECT(Rf_cons(head, tail)); SET_TAG(cell, obj); - // Update the head + tail to point at this cell. + // Update the head + tail to point at the newly-created cell, + // effectively inserting that cell between the current head + tail. SETCDR(head, cell); SETCAR(tail, cell); @@ -353,25 +354,29 @@ static struct { #endif } - void release(SEXP token) { - if (token == R_NilValue) { + void release(SEXP cell) { + if (cell == R_NilValue) { return; } #ifdef CPP11_USE_PRESERVE_OBJECT - R_ReleaseObject(token); + R_ReleaseObject(cell); return; #endif - SEXP head = CAR(token); - SEXP tail = CDR(token); + // Get a reference to the cells before and after the token. + SEXP lhs = CAR(cell); + SEXP rhs = CDR(cell); - if (head == R_NilValue && tail == R_NilValue) { + if (lhs == R_NilValue && rhs == R_NilValue) { Rf_error("should never happen"); } - SETCDR(head, tail); - SETCAR(tail, head); + // Remove the cell from the precious list -- effectively, we do this + // by updating the 'lhs' and 'rhs' references to point at each-other, + // effectively removing any references to the cell in the pairlist. + SETCDR(lhs, rhs); + SETCAR(rhs, lhs); } private: From e307721a05d5fda20c1d7a1504cb18fa42ca2bac Mon Sep 17 00:00:00 2001 From: Kevin Ushey Date: Sat, 17 Sep 2022 12:04:54 -0700 Subject: [PATCH 3/7] typo --- inst/include/cpp11/protect.hpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/inst/include/cpp11/protect.hpp b/inst/include/cpp11/protect.hpp index 86ac973e..16f79f6a 100644 --- a/inst/include/cpp11/protect.hpp +++ b/inst/include/cpp11/protect.hpp @@ -315,7 +315,7 @@ static struct { static SEXP list_ = get_preserve_list(); - // Get referneces to head, tail of the precious list. + // Get references to head, tail of the precious list. SEXP head = list_; SEXP tail = CDR(list_); @@ -368,10 +368,6 @@ static struct { SEXP lhs = CAR(cell); SEXP rhs = CDR(cell); - if (lhs == R_NilValue && rhs == R_NilValue) { - Rf_error("should never happen"); - } - // Remove the cell from the precious list -- effectively, we do this // by updating the 'lhs' and 'rhs' references to point at each-other, // effectively removing any references to the cell in the pairlist. From e402aa5b9ddb9f0da96727025d0271eac266bd47 Mon Sep 17 00:00:00 2001 From: Kevin Ushey Date: Sat, 17 Sep 2022 12:09:27 -0700 Subject: [PATCH 4/7] fix comments --- inst/include/cpp11/protect.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/inst/include/cpp11/protect.hpp b/inst/include/cpp11/protect.hpp index 16f79f6a..c0be6453 100644 --- a/inst/include/cpp11/protect.hpp +++ b/inst/include/cpp11/protect.hpp @@ -424,6 +424,6 @@ static struct { return preserve_list; } -} // namespace cpp11 -preserved; +} preserved; + } // namespace cpp11 From 8b40a3f9dd9e86ed3c8cf3a99fc831013fb417a9 Mon Sep 17 00:00:00 2001 From: Kevin Ushey Date: Sat, 17 Sep 2022 12:19:46 -0700 Subject: [PATCH 5/7] backwards compatibility? --- inst/include/cpp11/protect.hpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/inst/include/cpp11/protect.hpp b/inst/include/cpp11/protect.hpp index c0be6453..d16cb9f6 100644 --- a/inst/include/cpp11/protect.hpp +++ b/inst/include/cpp11/protect.hpp @@ -420,6 +420,8 @@ static struct { R_PreserveObject(preserve_list); set_preserve_xptr(preserve_list); } + if (CDR(preserve_list) == R_NilValue) + SETCDR(preserve_list, Rf_cons(R_NilValue, R_NilValue)); } return preserve_list; From 98f8ed80b47d7edf81033d15e0c1a8a6dd30c6e0 Mon Sep 17 00:00:00 2001 From: Kevin Ushey Date: Sat, 17 Sep 2022 12:30:13 -0700 Subject: [PATCH 6/7] code comments --- inst/include/cpp11/protect.hpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/inst/include/cpp11/protect.hpp b/inst/include/cpp11/protect.hpp index d16cb9f6..12063db8 100644 --- a/inst/include/cpp11/protect.hpp +++ b/inst/include/cpp11/protect.hpp @@ -411,21 +411,29 @@ static struct { } static SEXP get_preserve_list() { - static SEXP preserve_list = R_NilValue; + static SEXP preserve_list = R_NilValue; if (TYPEOF(preserve_list) != LISTSXP) { + preserve_list = get_preserve_xptr_addr(); if (TYPEOF(preserve_list) != LISTSXP) { preserve_list = Rf_cons(R_NilValue, Rf_cons(R_NilValue, R_NilValue)); R_PreserveObject(preserve_list); set_preserve_xptr(preserve_list); } + + // NOTE: Because older versions of cpp11 (<= 0.4.2) initialized the + // precious_list with a single cell, we might need to detect and update + // an existing empty precious list so that we have a second cell following. if (CDR(preserve_list) == R_NilValue) SETCDR(preserve_list, Rf_cons(R_NilValue, R_NilValue)); + } return preserve_list; + } + } preserved; } // namespace cpp11 From 4c36581e350aaa911c496cd965fb6d339300187e Mon Sep 17 00:00:00 2001 From: Kevin Ushey Date: Sat, 17 Sep 2022 12:38:39 -0700 Subject: [PATCH 7/7] satisfy grumpy formatter --- inst/include/cpp11/protect.hpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/inst/include/cpp11/protect.hpp b/inst/include/cpp11/protect.hpp index 12063db8..2948b494 100644 --- a/inst/include/cpp11/protect.hpp +++ b/inst/include/cpp11/protect.hpp @@ -411,10 +411,8 @@ static struct { } static SEXP get_preserve_list() { - static SEXP preserve_list = R_NilValue; if (TYPEOF(preserve_list) != LISTSXP) { - preserve_list = get_preserve_xptr_addr(); if (TYPEOF(preserve_list) != LISTSXP) { preserve_list = Rf_cons(R_NilValue, Rf_cons(R_NilValue, R_NilValue)); @@ -427,11 +425,9 @@ static struct { // an existing empty precious list so that we have a second cell following. if (CDR(preserve_list) == R_NilValue) SETCDR(preserve_list, Rf_cons(R_NilValue, R_NilValue)); - } return preserve_list; - } } preserved;