second pass at 'precious_{preserve,remove}' (addresses #382, #1081) #1133

Enchufa2 · 2021-01-17T13:13:09Z

This PR takes the work in the feature/rcpp_precious branch and modifies it to implement O(1) deletions as recommended by @ltierney in #1081. Reviews welcome.

Rcpp_PreserveObject takes an object, inserts the object in the precious list, and returns a tag.
Rcpp_ReleaseObject takes a tag and removes it from the list.
Rcpp_ReplaceObject has been removed.

Checklist

Code compiles correctly
R CMD check still passes all tests
Reprex in Rcpp is not using R_PreserveObject/R_ReleaseObject` as intended #1081 (comment) does not stall anymore.
Update file headers' copyright year.
Document the changes by file in ChangeLog.

Enchufa2 · 2021-01-17T13:53:36Z

I missed @eddelbuettel's first pass commit in my initial push. I recovered it and force-pushed.

eddelbuettel · 2021-01-17T14:39:12Z

Lovely! Will give it a whirl, a first Rcpp 1.0.6.1 reverse depends run (out of habit and prudence) just finished. Will give it a whirl. My initial attempt didn't fare too well. Hope this does better.

I missed @eddelbuettel's first pass commit in my initial push. I recovered it and force-pushed.

No worries. As long as a PR is clean... But appreciate it.

eddelbuettel · 2021-01-17T15:23:01Z

(But looking at git ls under an appropriate alias such as ls = log --color --graph --pretty=format:'%Cred%h%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr) %C(bold blue)<%an>%Creset' --abbrev-commit makes it look butt-ugly as we have wild branches overlapping. Should we consider scrapping this branch and cleanly applying the changes afresh from master, possibly just after #1132 is merged?)

In other news full reverse depends run now running, will report back in a day and a bit. No misses so far...

Enchufa2 · 2021-01-17T15:30:39Z

(But looking at git ls under an appropriate alias such as ls = log --color --graph --pretty=format:'%Cred%h%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr) %C(bold blue)<%an>%Creset' --abbrev-commit makes it look butt-ugly as we have wild branches overlapping. Should we consider scrapping this branch and cleanly applying the changes afresh from master, possibly just after #1132 is merged?)

Ok!

eddelbuettel · 2021-01-17T15:35:57Z

We may also be able to just rebase. To be seen once I merge the other.

eddelbuettel · 2021-01-17T17:12:08Z

It's interesting. The reverse depends check starts out as blood bath just like the last time. But with the recent experience of releasing 1.0.6 and even with the minor changes there seemingly requiring a rebuild of some other (binary) packages in the graph, I gave it a try in a "greenfield" setup freshly installing dependencies after the candidate package Rcpp 1.0.6.2 has been installed.

And that experiment is 3 for 3 so far. All the packages that failed with a "reused" .libPaths() now pass. So will require two runs, one before and after partial reinstallation. Add another day to it :)

Sadly, I wasn't that attentive last time so it is possible that the first pass at this wasn't as doomed as my glance at its reverse depends run suggested at first. Anyway ... thanks again for kicking that can further down the road with a refined PR :)

Enchufa2 · 2021-01-17T17:42:05Z

Unfortunately, requiring the new data member token in PreserveStorage, named_object<SEXP> and String breaks ABI compatibility. Therefore, I took the opportunity also to remove Rcpp_ReplaceObject, which doesn't fit in the new scheme anymore.

eddelbuettel · 2021-01-17T17:44:48Z

I didn't mention that none of these errors are typically as not as "in your face" as a "cannot compile" but rather come up later when tests are running. Altering the API surface clearly needs rebuilds, which is why we do it rarely. But there is something else much subtler going here with the ABI rather than API but it appears to mostly affect the packages built around stan which themselves to subsequent compilation.

@ltierney

with thanks to @ltierney for the report in RcppCore#1081 as well as a suggested alternative this branch reworks his idea somewhat to better fit how Rcpp sets itself up

eddelbuettel · 2021-01-18T17:20:24Z

So the reverse depends run turned into the anticipated "blood bath" -- see the summary -- but as mentioned, once packages are reinstalled it seems to pass.

After re-installing several hundred packages from source, a subsequent run is now working and looking better. A little over one hundred packages done and no errors yet (compared to around ten errors among the packages already tested). So this is looking good!

eddelbuettel · 2021-01-18T17:21:08Z

And I rebased from master with the just merged #1132 which looks cleaner too :)

Enchufa2 · 2021-01-18T20:40:58Z

Thanks for updating the ChangeLog. I didn't change the GPL copyright notice in the files' header because I'm not sure whether you prefer to just change the year, or to add a new one with a comma if they are not consecutive, or to add a new line in such a case. Also note that PreserveStorage.h has no copyright notice.

ChangeLog

eddelbuettel · 2021-01-18T20:46:51Z

Thanks for updating the changelog. I was lazy and dug up an older one. (And as noted, changing old one now is ... not what I'd encourage ... and you already correct, appreciate that)

Feel free to add you to file authorship if you made changes in the file. I also tend to update the end year to the current year.

Rev.dep still looking great. This will work. But I am still very confused why it needed such a comprehensive rebuild with several hundred packages needing a rebuild. It's a price worth paying, but I am a little, shall we say, "surprised" if not "worried" that I cannot assess when this is needed. Will also make the next CRAN upload more involved.

eddelbuettel · 2021-01-18T20:58:24Z

inst/include/Rcpp/String.h

@@ -3,6 +3,7 @@
 // String.h: Rcpp R/C++ interface class library -- single string
 //
 // Copyright (C) 2012 - 2018  Dirk Eddelbuettel and Romain Francois
+// Copyright (C) 2021  Iñaki Ucar


I would probably have made that

// Copyright (C) 2012 - 2020 Dirk Eddelbuettel and Romain Francois
// Copyright (C) 2021 Dirk Eddelbuettel and Romain Francois and Iñaki Ucar

which is for example what we have done here (from a quick grep)

Environment.h:// Copyright (C) 2009 - 2013 Dirk Eddelbuettel and Romain Francois
Environment.h:// Copyright (C) 2014 - 2020 Dirk Eddelbuettel, Romain Francois and Kevin Ushey
[...]
exceptions_impl.h:// Copyright (C) 2012 - 2019 Dirk Eddelbuettel and Romain Francois
exceptions_impl.h:// Copyright (C) 2020 Dirk Eddelbuettel, Romain Francois, and Joshua N. Pritikin

Thoughts?

eddelbuettel · 2021-01-18T20:59:15Z

inst/include/Rcpp/storage/PreserveStorage.h

@@ -1,3 +1,25 @@
+// -*- mode: C++; c-indent-level: 4; c-basic-offset: 4; indent-tabs-mode: nil; -*-


And these I no longer add as we switched to relying on editorconf

eddelbuettel · 2021-01-18T21:00:18Z

Thanks for the header commit. I put some small nits up above and may just do a quick cleanup commit in a few. Ok with you?

Enchufa2 · 2021-01-18T21:29:03Z

Sure!

eddelbuettel

LGTM -- just awaiting a second rev.deps run for completeness

eddelbuettel · 2021-01-19T14:52:56Z

Second pass done and looking good (see RcppCore/rcpp-logs@eec49e3) so merging.

kevinushey · 2021-01-19T19:03:32Z

Awesome work -- thanks for putting this together (and especially taking the time to tie up all the loose ends in the String class).

Sorry for coming to this late, but one comment: There may be packages using Rcpp that directly use Rcpp_PreserveObject() and Rcpp_ReleaseObject(). Is there a way to check if such packages exist? If they do, I think such packages might be in trouble as they would now be leaking the R objects they protect (since the old usages of the API don't know about the tokens being used, and they would then be trying to protect + unprotect the object directly rather than via the token).

Out of an abundance of caution, I would've preferred if we maintained the old API for backwards compatibility just in case, but moved to using a separate internal API for protecting + releasing Rcpp objects (using our new precious list). But we might want to defer that work until we have evidence that this is a real issue; and we could also take the stance that package authors shouldn't be using internal Rcpp APIs anyhow.

tl;dr: I think we should maintain the old implementations here:

Rcpp/inst/include/RcppCommon.h

Lines 94 to 100 in 0ba1b55

    
           inline SEXP Rcpp_PreserveObject(SEXP object) { 
        
               return Rcpp_precious_preserve(object); 
        
           } 
        
           inline void Rcpp_ReleaseObject(SEXP token) { 
        
               Rcpp_precious_remove(token); 
        
           }

but use separate Rcpp_PreciousPreserve() and Rcpp_PreciousRelease() APIs for protecting objects via Rcpp's precious list, and "eventually" encourage users who really want to manage protection manually to use those APIs.

eddelbuettel · 2021-01-19T19:07:48Z

Good points. The 2200+ packages in the reverse depends set (essentially: all CRAN rev.deps minus the exclusion we keep) did not mind but I agree in principle. Not a single 'did not build issue'. But you are correct that we could maintain the older API surface to the extend possible.

[ As an aide I am still confused as hell why some many packages (largely, though I think not exclusively, in the rstan nexus) needed a rebuild and a little lost as to why we are unable to anticipate this. ]

eddelbuettel · 2021-01-19T20:14:39Z

I just pushed a new branch feature/preserve_interface along those lines.

kevinushey · 2021-01-19T20:52:15Z

Took a quick look; looks good to me. Thanks for jumping on it so quickly!

Enchufa2 force-pushed the feature/rcpp_precious_2 branch from eb6b219 to 0829beb Compare January 17, 2021 13:52

eddelbuettel force-pushed the feature/rcpp_precious_2 branch from f52bb4b to 079903a Compare January 17, 2021 15:09

eddelbuettel and others added 3 commits January 18, 2021 11:13

first pass at 'precious_{preserve,remove}'

519909c

with thanks to @ltierney for the report in RcppCore#1081 as well as a suggested alternative this branch reworks his idea somewhat to better fit how Rcpp sets itself up

second pass at 'precious_{preserve,remove}'

21ddbb9

roll minor version given RcppCore#1132

75eb049

eddelbuettel force-pushed the feature/rcpp_precious_2 branch from 079903a to 75eb049 Compare January 18, 2021 17:21

eddelbuettel and others added 2 commits January 18, 2021 13:14

add ChangeLog entries

7a207bf

update email

5dc58ac

eddelbuettel reviewed Jan 18, 2021

View reviewed changes

ChangeLog Outdated Show resolved Hide resolved

revert email in old entries

9cc9239

update headers' copyright notice

bc48e46

eddelbuettel reviewed Jan 18, 2021

View reviewed changes

minor spit and polish to some copyright headers

2c2eb89

eddelbuettel approved these changes Jan 18, 2021

View reviewed changes

eddelbuettel merged commit 7f56d2e into RcppCore:master Jan 19, 2021

This was referenced Jan 19, 2021

Rcpp is not using R_PreserveObject/R_ReleaseObject` as intended #1081

Closed

provide better protection mechanism than R_PreserveObject / R_ReleaseObject? #382

Closed

eddelbuettel added a commit that referenced this pull request Jan 19, 2021

updated NEWS for #1132 and #1133

0ba1b55

eddelbuettel added a commit that referenced this pull request Jan 19, 2021

updated NEWS for #1132 and #1133

4ed72aa

eddelbuettel mentioned this pull request Jan 19, 2021

maintain existing interface and offer new one #1135

Merged

4 tasks

Enchufa2 deleted the feature/rcpp_precious_2 branch January 20, 2021 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

second pass at 'precious_{preserve,remove}' (addresses #382, #1081) #1133

second pass at 'precious_{preserve,remove}' (addresses #382, #1081) #1133

Enchufa2 commented Jan 17, 2021 •

edited

Loading

Enchufa2 commented Jan 17, 2021

eddelbuettel commented Jan 17, 2021 •

edited

Loading

eddelbuettel commented Jan 17, 2021 •

edited

Loading

Enchufa2 commented Jan 17, 2021

eddelbuettel commented Jan 17, 2021

eddelbuettel commented Jan 17, 2021

Enchufa2 commented Jan 17, 2021

eddelbuettel commented Jan 17, 2021

eddelbuettel commented Jan 18, 2021

eddelbuettel commented Jan 18, 2021

Enchufa2 commented Jan 18, 2021

eddelbuettel commented Jan 18, 2021

eddelbuettel Jan 18, 2021

eddelbuettel Jan 18, 2021

eddelbuettel commented Jan 18, 2021

Enchufa2 commented Jan 18, 2021

eddelbuettel left a comment

eddelbuettel commented Jan 19, 2021

kevinushey commented Jan 19, 2021

eddelbuettel commented Jan 19, 2021

eddelbuettel commented Jan 19, 2021

kevinushey commented Jan 19, 2021

		@@ -1,3 +1,25 @@
		// -- mode: C++; c-indent-level: 4; c-basic-offset: 4; indent-tabs-mode: nil; --

second pass at 'precious_{preserve,remove}' (addresses #382, #1081) #1133

second pass at 'precious_{preserve,remove}' (addresses #382, #1081) #1133

Conversation

Enchufa2 commented Jan 17, 2021 • edited Loading

Checklist

Enchufa2 commented Jan 17, 2021

eddelbuettel commented Jan 17, 2021 • edited Loading

eddelbuettel commented Jan 17, 2021 • edited Loading

Enchufa2 commented Jan 17, 2021

eddelbuettel commented Jan 17, 2021

eddelbuettel commented Jan 17, 2021

Enchufa2 commented Jan 17, 2021

eddelbuettel commented Jan 17, 2021

eddelbuettel commented Jan 18, 2021

eddelbuettel commented Jan 18, 2021

Enchufa2 commented Jan 18, 2021

eddelbuettel commented Jan 18, 2021

eddelbuettel Jan 18, 2021

Choose a reason for hiding this comment

eddelbuettel Jan 18, 2021

Choose a reason for hiding this comment

eddelbuettel commented Jan 18, 2021

Enchufa2 commented Jan 18, 2021

eddelbuettel left a comment

Choose a reason for hiding this comment

eddelbuettel commented Jan 19, 2021

kevinushey commented Jan 19, 2021

eddelbuettel commented Jan 19, 2021

eddelbuettel commented Jan 19, 2021

kevinushey commented Jan 19, 2021

Enchufa2 commented Jan 17, 2021 •

edited

Loading

eddelbuettel commented Jan 17, 2021 •

edited

Loading

eddelbuettel commented Jan 17, 2021 •

edited

Loading