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.

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

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

Merged
merged 8 commits into from
Jan 19, 2021

Conversation

Enchufa2
Copy link
Member

@Enchufa2 Enchufa2 commented Jan 17, 2021

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

@Enchufa2
Copy link
Member Author

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

@eddelbuettel
Copy link
Member

eddelbuettel commented Jan 17, 2021

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
Copy link
Member

eddelbuettel commented Jan 17, 2021

(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
Copy link
Member Author

(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
Copy link
Member

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

@eddelbuettel
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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.

eddelbuettel and others added 3 commits January 18, 2021 11:13
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
Copy link
Member

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
Copy link
Member

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

@Enchufa2
Copy link
Member Author

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 Outdated Show resolved Hide resolved
@eddelbuettel
Copy link
Member

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.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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?

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

Choose a reason for hiding this comment

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

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

@eddelbuettel
Copy link
Member

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
Copy link
Member Author

Sure!

Copy link
Member

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

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

@eddelbuettel
Copy link
Member

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

@kevinushey
Copy link
Contributor

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:

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
Copy link
Member

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 added a commit that referenced this pull request Jan 19, 2021
@eddelbuettel
Copy link
Member

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

@kevinushey
Copy link
Contributor

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

@Enchufa2 Enchufa2 deleted the feature/rcpp_precious_2 branch January 20, 2021 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants