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

[ltierney]

This is based on an issue found by @bhive01 and discussed with him
and @clauswilke in wilkelab/gridtext#9.

Running the code included at the end in R versions up through 4.0.0
with the C stack set to 1M will segfault with a C stack overflow. The
same happens (for me on Ubuntu at least) with the default 8M stack if
you change the value of N to 1000.

Running this code as is (i.e. N == 500) and with an 8M stack will not
fail, but takes 14 minutes due to several multi-minute-long pauses in
the second loop.

The problem is that this code, via calls to R_PreserveObject in
Rcpp, puts around 300K objects into the preserved object list. This is
a simple linked list, so has to be searched linearly to remove objects
pushed on early. That is the reason for the long pauses. The segfault
happens because the code for R_ReleaseObject was using recursion to
work down the list. R-devel and R-patched now use iteration and no
longer segfault, but that doesn't help anyone running R 4.0.0 or
earlier.

The _R Extensions Manual_ says about the
R_PreserveObject/R_ReleaseObject mechanism:

It is less efficient than the normal protection mechanism, and
should be used sparingly.

The linked list data structure, and even the recursive delete, are
reasonable for this usage but are not suited for handling 300K objects
that may need to be removed in random or first in/first-out order.

Assuming you need to keep alive R objects, a better way to do this is
to create an appropriate data structure that you control, and protect
only that data structure with R_PreserveObject (and maybe release it
on unload with R_ReleseObject). That way you can use a more
appropriate data structure, such as a hash table. Using a hash table
the code that currently takes 14 minutes would only take about 1.5
minutes. The improvement is even larger for N == 1000.

Even if you don't want to go to a hash table now it would be a good
idea to switch to using your own list so users of R 4.0.0 or earlier
don't run the risk of a segfault.

For using your own list you need something like this

static SEXP Rcpp_precious = NULL;

void R_init_Rcpp(DllInfo* dllinfo) {
    Rcpp_precious = CONS(R_NilValue, R_NilValue);
    R_PreserveObject(Rcpp_precious);
   ...
}

Then replace R_PreserveObject and R_ReleaseObject with

void Rcpp_PreserveObject(SEXP object)
{
    SETCDR(Rcpp_precious, CONS(object, CDR(Rcpp_precious)));
}

void Rcpp_ReleaseObject(SEXP object)
{
    SETCDR(Rcpp_precious, DeleteFromList(object, CDR(Rcpp_precious)));
}

where (this is now in R_devel/R_patched)

static SEXP DeleteFromList(SEXP object, SEXP list)
{
    if (CAR(list) == object)
        return CDR(list);
    else {
        SEXP last = list;
        for (SEXP head = CDR(list); head != R_NilValue; head = CDR(head)) {
            if (CAR(head) == object) {
                SETCDR(last, CDR(head));
                return list;
            }
            else last = head;
        }
        return list;
    }
}

This maintains your own list, which is protected by being in the CDR
field of the protected Rcpp_precious list cell. This will give you
the same reliability and performance across all R versions. [Code is
not tested, so there may be typos, but that is design I recommend].

If you want better performance for a use pattern as exhibited in the
example here you could look at using a hash table stored in a VECSXP
placed in one of the cells of Rcpp_precious. In a quick and dirty
experiment a simple hash table did much better, but the code is of
course more complicated.

All that said, I have a feeling that it should be possible to do
better by using a combination of the protected fields in external
pointer objects and weak references. If you have a document describing
the design of your memory management I could take a look and see if I
can suggest something.

Here is the code to run, from @clauswilke in wilkelab/gridtext#9.

library(ggplot2)
library(ggtext)
library(grid)

plot_grob <- function(df) {
  p <- ggplot(data.frame(x = 1, y = 1)) +
    geom_point(aes(x, y)) +
    labs(caption = "<br>Pink dots represent outliers and are removed from downstream analyses.<br>Error bars represent the standard error of the mean.<br>ANOVA model significance *p* < 0.05.<br>Treatments with the same letter are not significantly different at *α* = 0.05 according to Tukey's HSD. ") + 
    theme(plot.caption =  element_textbox_simple())
  ggplotGrob(p)
}

N <- 500

l <- list()
for (i in 1:N) {
  cat(i, " ")
  g <- plot_grob()
  grid.newpage()
  grid.draw(g)
  l[[i]] <- g
}

l <- NULL
l <- list()
for (i in 1:N) {
  cat(i, " ")
  g <- plot_grob()
  grid.newpage()
  grid.draw(g)
  l[[i]] <- g
}

[eddelbuettel]

Thanks for the detailed post, and the worked suggestion using Rcpp_precious (love the name ;-) ) and a simple list. For disruptive changes I appreciate the minimalism. If this works (as expected) we could look into a hashed structure later.

Now, is there by chance a truly _minimally_ reproducible example that would not involve the ggplot2 stack, tibbles, and a number of other packages (including one not on CRAN) ?

[ltierney]

On Sun, 17 May 2020, Dirk Eddelbuettel wrote:

Thanks for the detailed post, and the worked suggestion using Rcpp_precious
(love the name ;-) ) and a simple list. For disruptive changes I appreciate
the minimalism. If this works (as expected) we could look into a hashed
structure later.

Now, is there by chance a truly minimally reproducible example that would
not involve the ggplot2 stack, tibbles, and a number of other packages?

Not from me. I don't know if there is something these other packages
are doing that from Rcpp's perspective they shouldn't, but that is
between you and them. Rcpp is making a ton of calls to
R_PreserveObject, and that isn't the way that mechanism is intended to
be used.

It does look from your NEWS file like there is already some work in
progress in this direction; maybe it would be good to accelerate that.

But other than looking over a design document, if you have one, I'm
stepping back from this.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, orunsubscribe.[AA55UVCUQJORGFQC64J2S2TRSABDHA5CNFSM4NDNY5UKYY3PNVWWK3TUL52HS4
DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEWFEHTQ.gif]

--
Luke Tierney
Ralph E. Wareham Professor of Mathematical Sciences
University of Iowa Phone: 319-335-3386
Department of Statistics and Fax: 319-335-3017
Actuarial Science
241 Schaeffer Hall email: [email protected]
Iowa City, IA 52242 WWW: http://www.stat.uiowa.edu

[eddelbuettel]

Use of R_PreserveObject (and the matching release) goes back 15 years to the very first C++ class wrappers around SEXP objects. By using C++ facilities like the constructor and destructor we can generally ensure proper resource management (cf the RTTI idiom in C++ parlance).

And we rather happily relied on this base R feature all those years.

But presumably _all_ use cases up to now were only 'lower-case s' stress testing where @clauswilke and @bhive01 seem to have found a way to put a capital S there. Or, quite possibly, other use cases that may have seen failures were less determined to drill down. You all now did, so thanks for that.

Resource management in the context of a dynamically managed system can have its challenges. Relying on this mechanism worked really well so far. Let's see if you can turn it up one notch. Looking at the code now, requires some tweaks.

[ltierney]

On Sun, 17 May 2020, Dirk Eddelbuettel wrote:

Use of R_PreserveObject (and the matching release) goes back 15 years to the
very first C++ class wrappers around SEXP objects. By using C++ facilities
like the constructor and destructor we can generally ensure proper resource
management (cf the RTTI idiom in C++ parlance).

And we rather happily relied on this base R feature all those years.

But presumably all use cases up to now were only 'lower-case s' stress
testing where @clauswilke and @bhive01 seem to have found a way to put a
capital S there. Or, quite possibly, other use cases that may have seen
failures were less determined to drill down. You all now did, so thanks for
that.

Same for the 20-or-so-year old recursive code in
R_ReleaseObject. Works fine until someone throws 300K objects at it
and tries to release the first one ...

Resource management in the context of a dynamically managed system can have
its challenges. Relying on this mechanism worked really well so far. Let's
see if you can turn it up one notch. Looking at the code now, requires some
tweaks.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, orunsubscribe.[AA55UVBLSGYEJICAC7SUZHTRSAGFLA5CNFSM4NDNY5UKYY3PNVWWK3TUL52HS4
DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEWFFYHY.gif]

--
Luke Tierney
Ralph E. Wareham Professor of Mathematical Sciences
University of Iowa Phone: 319-335-3386
Department of Statistics and Fax: 319-335-3017
Actuarial Science
241 Schaeffer Hall email: [email protected]
Iowa City, IA 52242 WWW: http://www.stat.uiowa.edu

[kevinushey]

This would probably function as a reproducible example (depending on stack size + n). Save this to a file and then call Rcpp::sourceCpp("file.cpp"):

`````

include

using namespace Rcpp;

// [[Rcpp::export]]
void barf(std::size_t n) {

std::vector objects(n);
for (int i = 0; i < n; i++)
objects[n - i - 1] = Rf_ScalarInteger(i);

// destructor for 'objects' runs: uh oh?
}

/* R
barf(1E6L)
*/
`````

(Note: reproducibility may depend on whether your C++ standard library implementation destructs elements from first-to-last, or last-to-first. AFAIK this isn't explicitly mandated by the standard)

The problem occurs because of order of destruction for vector elements doesn't match the order in which they were created, leading to this deep recursion into the precious list.

Ultimately, the goal of the Rcpp constructors and destructors is to protect the associated object from the GC for the lifetime of that object, and unprotect it once collection is safe.

I think using a hash table is worth considering just because it would insulate us from other issues (e.g. performance) related to order-of-destruction issues.

[eddelbuettel]

Same for the 20-or-so-year old recursive code in R_ReleaseObject.

Correct, and we were on the same calling path.

[eddelbuettel]

While the suggested design can be added to the package fairly easily, it does not account for use from sourceCpp() et al so this will need a refinement. Not as quick a change as I had hoped.

[clauswilke]

I have been trying to create a simpler reprex, but I'm not sure I've been successful. The code below attempts to simulate what I think is causing the issues in the ggplot2 example, and it crashes my R session if I run it twice. But the error is segfault: 'memory not mapped', which seems different from stack overflow. Posting it here in case it's helpful.

#include <Rcpp.h>
#include <vector>
using namespace Rcpp;

class A {
  std::vector<Rcpp::String> s;
public:
  A() {
    for (int i = 0; i <50000; i++) {
      s.push_back("A");
    }
  }
};

// [[Rcpp::export]]
XPtr<A> test() {
  XPtr<A> a(new A());
  return a;
}

/*** R
N <- 1000

l <- list()
for (i in 1:N) {
  cat(i, " ")
  l[[i]] <- test()
}

l <- list()
for (i in 1:N) {
  cat(i, " ")
  l[[i]] <- test()
}
*/

[eddelbuettel]

(But that is slightly different. XPtr means "user knows what he is doing, we leave it alone". When you as a user pick one and allocate with via new, you are also either supposed to delete (or ensure it is done for you). Plus we're now entering a whole new element to the story by adding an STL vector too.)

[clauswilke]

Ah, Ok. I'm the user here, but I may not know what I'm doing. :-) The gridtext package definitely wraps things into XPtrs. The STL vector may not be needed for the example. I'll try to refine the reprex.

[clauswilke]

@eddelbuettel Could you provide a minimal correct example using XPtr? How does the C++ code know when the R object holding the pointer goes out of scope? I always assumed delete is called automatically when the last reference to the pointer disappears.

[eddelbuettel]

Sorry, I need to rephrase that. The XPtr generally has a default finaliser too so I stated that poorly, my bad. I use them a lot myself when I also rely on a default cleanup. It's just that _maybe_ in this example ensuring a cleanup at end of scope may help. To be seen.

[clauswilke]

In my reprex, you could just replace std::vector<Rcpp::String> s; with Rcpp::StringVector s; to avoid the STL. It just gets very slow, and I'm not sure it triggers the stack overflow either.

[eddelbuettel]

Simplifying is good. And apologies again for the comment two up---I also rely on the default finalizer (see inst/include/Rcpp/XPtr.h but that is of course gnarly code -- key is standard_delete_finalizer() ). In your example, though. you write to the same long list _twice_ overwriting elements. Methinks those are then lost and never cleaned so that looks like a code smell to me.

Other (standard) XPtr use (I do a ton myself) is to ... do some work, set up some data structures or pointers to persist ... do some other stuff ... have a code block relying on the XPtr allocated objects ... some other things ... and then end-of-scope (end of subroutine say) to clean things up. That works of course as intended.

"It just gets very slow": Yes, we always say that R objects we wrap should not use push_back() because by the semantics of R vectors each of those is a full copy. So to that extend you were right in that code piece using a STL vector. Anyway, maybe we can go back the example by @kevinushey .

[eddelbuettel]

Ok, needed to take a break, clear my head, work on something else ... to then come back and now have the (basic idea and solution of) Luke's proposal implemented (in a slightly different way because of the way Rcpp is called etc pp). Still passes unit tests, but (once I installed package ggtext) it does not appear to run the example above any better: it still slows to halt in loop and then spends minutes sitting there. (R 4.0.0, Ubuntu 19.10, everything current from CRAN).

[ltierney]

y1;5202;0cOn Sun, 17 May 2020, Dirk Eddelbuettel wrote:

Ok, needed to take a break, clear my head, work on something else ... to
then come back and now have the (basic idea and solution of) Luke's proposal
implemented (in a slightly different way because of the way Rcpp is called
etc pp). Still passes unit tests, but (once I installed package ggtext) it
does not appear to run the example above any better: it still slows to halt
in loop and then spends minutes sitting there. (R 4.0.0, Ubuntu 19.10,
everything current from CRAN).

If you are using a linked list then that is what I would expect. But
you should have gotten rid of the stack overflow segfault that you
would still be getting in R 4.0.0 (you would need R-devel/R-patched to
avoid that without doing something in Rcpp).

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, orunsubscribe.[AA55UVC3KZVXQQWLAUFH2ADRSBLXXA5CNFSM4NDNY5UKYY3PNVWWK3TUL52HS4
DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEWFP34A.gif]

--
Luke Tierney
Ralph E. Wareham Professor of Mathematical Sciences
University of Iowa Phone: 319-335-3386
Department of Statistics and Fax: 319-335-3017
Actuarial Science
241 Schaeffer Hall email: [email protected]
Iowa City, IA 52242 WWW: http://www.stat.uiowa.edu

[eddelbuettel]

It never crashed for me before or after. R 4.0.0, standard Ubuntu build via Michael Rutter based on my standard Debian package for R. No tricks, no patches. Also Ubuntu 19.10, standard kernel, but more memory than I had on past machines but nothing outrageous.

I may push this branch later for @kevinushey to look at (and/or @clauswilke to try too).

[s-u]

@kevinushey to understand the conditions elaborating on your example: one would hope that this is not something that anyone would actually write, since all the protection is unnecessary and wasteful. Simply allocating an R list would need only a single protection for the list itself and no other protection as all elements are already protected by the enclosing object. So is there any hope of identifying where those excessive preserve/release calls come from? I'd say that is really where the problem lies - R package code almost never needs explicit protections, since only the result object needs to be protected and all other objects are transient with direct transfer of ownership. I think Rcpp would be in a great position to provide tools here - it could warn if the pool gets large. (This is assuming that Rcpp itself is not creating those unnecessary protections and that it provides proper ownership transfer when assigning R objects.)

[eddelbuettel]

Step back a second: Rcpp lets you create object, so we simply started 'by setting the protect bit' to make them comply with standard behavior. That worked wonderfully well since the very beginning, and this is really the first report I can think of identifying side effects. And with that, again thanks for already suggesting one alternative.

And the thought of a singleton instance hosting such a 'pool', possibly via hashing, also crossed my mind today. We'll see if anybody has appetite and time to work on it.

[s-u]

I'm not sure I understand - there is no "protect bit" in R - there is only object ownership. If an object is not contained in (owned by) any other object, it is considered deletable and gets garbage-collected - that's how R works. There are two objects are that never collected: the protect stack and the precious list. The former is where PROTECT() puts objects and latter where Protect/Release puts objects. So the only way you ensure that an object is not garbage-collected is that you assign it inside any object that is protected. In R packages this is typically done by allocating the result object protecting it (typically on the protection stack which is faster) for the duration of the call and assigning any newly allocated object to the result object. This way you don't need any additional protection since the result object is already protected - that's why in the R API you can simply do SET_VECTOR_ELT(res, i, allocVector(...)) without the need for any protection. I would expect that the equivalent in Rcpp does the same.

As Luke said, if you had any official description of the Rcpp memory management model we may help looking into it. There are different memory models - for anything R-related I'd say the model where assigning an object into a container transfers ownership is the most appropriate - even if you did perform any unnecessary protection, it would be likely a very recent object since you release it on the spot. It would make sense for Rcpp to implement its own protection mechanism if it matched the usage better - e.g. you could have a hierarchical pool to match the function stack etc. It could even make its own decisions on when to protect or not based on the objects it knows about - the abstraction really gives it a lot of flexibility.

[eddelbuettel]

It's not very complicated, and the sources are out there for everybody to look at. R_PreserveObject() is what we used since at least 0.7.0 in 2009 (which I just spot-checked). It is making use of the API you and the rest of R Core provide.

[kevinushey]

Taking a closer look, this code looks suspect:

https://github.com/RcppCore/Rcpp/blob/5e5de186fbc923293136689fa889342b0c31a887/inst/include/Rcpp/String.h#L530-L538

Normally, wrap() should return a raw, unprotected SEXP for the user. The fact that we're preserving the object implies that the returned object is effectively 'leaked' and won't be cleaned up (since the user shouldn't ever call R_ReleaseObject() on something returned by wrap()).

So I think this may ultimately be a bug in the Rcpp String implementation. Here's a way to reproduce with (using sourceCpp()):

`````

include

using namespace Rcpp;

// [[Rcpp::export]]
SEXP barf(std::size_t n) {

// create some protected object
SEXP myObject = Rf_ScalarReal(42.0);
R_PreserveObject(myObject);

// create and wrap a bunch of String objects
// call wrap() to trigger protection leak bug
SEXP object;
for (int i = 0; i < n; i++) {
    String string;
    object = wrap(string);
}

// try and release previous object -- stack overflow
R_ReleaseObject(myObject);
return object;

}

/* R
barf(1E6)
*/
`````

[eddelbuettel]

Hm, OTOH was that not exactly the standard pattern: mark an object returned to R as protected?

I just cleaned up the branches; I had accidentally dropped the earlier commit on master which was of course a mistake (reset and force pushed, master back to where it was) and the work based on Luke's suggestion in its own branch.

[kevinushey]

wrap() should return a raw, unprotected SEXP, which is then usually used in one of these ways:

1. It is returned immediately; e.g. for functions that return a SEXP (normally called via .Call());
2. It is protected on the stack, e.g. via PROTECT() or the Rcpp equivalent Shield<SEXP>,
3. It is re-preserved e.g. by assigning to a new Rcpp object.

The String wrap() method really is the odd one out here. Compare that with e.g.

https://github.com/RcppCore/Rcpp/blob/95d0854f2102e0a4aaba888e4c312e0b1c015297/inst/include/Rcpp/internal/wrap.h#L929-L934

which really does just give you an unprotected R object from a const char*.

[eddelbuettel]

Hm, indeed, point taken.

Sadly, "empirically" on the problem by @clauswilke none of it matters. Still runs ~15 mins for me with all the slowdowns in the 2nd run. Using the code in the new branch.

[s-u]

I have instrumented R[1] to record modifications of the precious list and their source. The result is that @clauswilke's example for N=100 does 805,500 calls to Rcpp_PreserveObject() and 783,052 Rcpp_ReleaseObject()calls (after removing all objects and gc()), so the main issue here is definitely a leak leaving 22,448 objects on the list (the leak grows with each iteration). I did the proposed change by @kevinushey to Rcpp but it didn't seem to have an effect.

While analyzing the log, one thing that struck me is that objects get protected many times - out of the 805,500 protections 517,299 are duplicates (i.e. protecting already protected object).

Here is the breakdown of multiplicity (i.e. how many entries of the object are still left on the precious list) and type of the leaked objects for that example with N=100:

       STR  VEC EXTPTR
  1   4200  206 9142
  84     0  100    0
  500    1    0    0

As I was trying to point out, Rcpp is actually in a position to track such things itself and address it and/or provide guidance to package authors since it controls the abstraction. This has really nothing to with R, but with the memory management model of Rcpp.

[1] - https://gist.github.com/s-u/68423081a8ba9b33765f3742f75ea890

[eddelbuettel]

These tests are slow, and I didn''t quite internalize how these choice of preserving/removing objects affect packages ggtext (and/or gridtext) -- so I finally switched to rebuilding them after trying different Rcpp options.

And for now it looks like the branch by @kevinushey wins. The example runs in about 10 minutes, so faster than the old solution and faster than the suggested mod by @ltierney which I put in first.

Ultimately we probably want something else. To be discussed.

[s-u]

I was digging deeper and I really didn't realize that we're essentially just debugging gridtext:

   2200 String() gridtext
 808000 set__() gridtext
 776152 ~PreserveStorage() gridtext
   2200 ~String() gridtext

Note that the PR above has no effect on the leaks for me. It is certainly sensible, just doesn't seem to reduce the allocations in gridtext.

[clauswilke]

To be clear: There may very well be problems in the gridtext code. We may also be dealing with multiple different issues at once. The original problem was a stack overflow (https://github.com/wilkelab/gridtext/issues/9), and we determined that that was not under my control. If gridtext is leaking memory, that would be a separate issue.

[eddelbuettel]

The example runs in about 10 minutes

Sorry, most of that speed gain was a different test in the 'bug' script where I replaced l <- list() with l <- vector(mode="list", length=N). Without it, time taken is again about the same.

[eddelbuettel]

(The commit message by @kevinushey was overeagerly interepreted by GH's bot as a fixed. Reopening).

PR #1082 is now in Rcpp 1.0.4.11 which you can install from the drat repo if desired: install.packages("Rcpp", repos="https://rcppcore.github.io/drat")

[kevinushey]

Regarding the memory model -- Rcpp's objects are basically like C++ shared_ptrs, in that multiple Rcpp objects can reference the same R object, and that object should remain alive (protected from garbage collection) as long as any Rcpp object referencing that data is alive.

Given that, we could probably do a lot better by using our own internal reference counting implementation, and only preserve / release objects when the reference count changes between 0 and 1.

I agree it also makes sense to use our own hash table rather than pushing everything directly onto the R_PreciousList.

[ltierney]

On Mon, 18 May 2020, Kevin Ushey wrote:

Regarding the memory model -- Rcpp's objects are basically like C++
shared_ptrs, in that multiple Rcpp objects can reference the same R object,
and that object should remain alive (protected from garbage collection) as
long as any Rcpp object referencing that data is alive.

Given that, we could probably do a lot better by using our own internal
reference counting implementation, and only preserve / release objects when
the reference count changes between 0 and 1.

I agree it also makes sense to use our own hash table rather than pushing
everything directly onto the R_PreciousList.

An alternative that might be worth exploring (not thought through
carefully so there may be issues):

Instead of, or in addition to, storing the R SEXP in your Rccp object,
store a CONS cell with the R SEXP in the TAG field. Use the CAR and
CDR fields to link the cell into a doubly-linked list, with the head
registered with R_PreservObject. When there are no longer Rccp
references to the R SEXP, the point where you are now using
R_DeleteObject, just unlink the cell. That would give O(1)
performance.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, orunsubscribe.[AA55UVCYLLFHSQ5IOFGSIZDRSFR5JA5CNFSM4NDNY5UKYY3PNVWWK3TUL52HS4
DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEWI4K2Q.gif]

--
Luke Tierney
Ralph E. Wareham Professor of Mathematical Sciences
University of Iowa Phone: 319-335-3386
Department of Statistics and Fax: 319-335-3017
Actuarial Science
241 Schaeffer Hall email: [email protected]
Iowa City, IA 52242 WWW: http://www.stat.uiowa.edu

[stephematician]

Regarding the memory model -- Rcpp's objects are basically like C++ shared_ptrs, in that multiple Rcpp objects can reference the same R object, and that object should remain alive (protected from garbage collection) as long as any Rcpp object referencing that data is alive.

If this is the case, does managing the resource via a shared_ptr with custom alloc/finaliser make sense - as opposed to implementing your own reference counting?

[kevinushey]

If this is the case, does managing the resource via a shared_ptr with custom alloc/finaliser make sense - as opposed to implementing your own reference counting?

I don't think this helps. Rcpp objects just have a reference to the underlying SEXP, and need to preserve that object in the constructor and release that object in the destructor (via RAII). Unless I'm missing something, using a shared pointer with a custom finalizer would just be a roundabout way of accomplishing the same thing.

[stephematician]

If this is the case, does managing the resource via a shared_ptr with custom alloc/finaliser make sense - as opposed to implementing your own reference counting?

I don't think this helps. Rcpp objects just have a reference to the underlying SEXP, and need to preserve that object in the constructor and release that object in the destructor (via RAII). Unless I'm missing something, using a shared pointer with a custom finalizer would just be a roundabout way of accomplishing the same thing.

I mean if you construct the (Rcpp) object from a SEXP you

• call protect or preserve, and
• store the pointer as a shared_ptr with a finalizer that unprotects or releases.

Then move or copy of an Rcpp object doesnt need to call protect or preserve on the SEXP, just move/copy the shared_ptr and it will take care of the ref counting.

[eddelbuettel]

Hi @clauswilke I see per CRANberries that ggtext is now on CRAN -- congrats. Did the issue discussed here affecting interactions between ggtext, gridtext, Rcpp and R get sorted out or circumvented? Was there anything we can learn from it?

[bhive01]

Dirk, I can tell you as the end-user of ggtext/gridtext and the originator
of this issue that Luke's edits to R-devel solved the crashing problem, but
at significant cost to run-time under non-crashing circumstances. I don't
understand enough of the underlying systems to comment on how to fix the
performance, but if any of the comments above can help in that regard I
would certainly welcome those improvements as well.

On Thu, Jun 4, 2020 at 5:50 AM Dirk Eddelbuettel [e-mail]
wrote:

Hi @clauswilke https://github.com/clauswilke I see per CRANberries that ggtext
is now on CRAN
http://dirk.eddelbuettel.com/cranberries/2020/06/04#ggtext_0.1.0 --
congrats. Did the issue discussed here affecting interactions between
ggtext, gridtext, Rcpp and R get sorted out or circumvented? Was there
anything we can learn from it?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/RcppCore/Rcpp/issues/1081#issuecomment-638826997, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AADANXKBWOIHELZPGSFJX6DRU6KCTANCNFSM4NDNY5UA
.

[ltierney]

The performance issue is well understood -- it is the linear search for the object to be removed.

At this point I don't think R-core can afford to do anything about that, as performance is fine if the facility is used as intended (i.e._sparingly_). There is an outside chance that an improvement might come as part of other improvements, but I wouldn't count on it.

If Rccp has not resolved this yet, I am fairly confident that the doubly-linked list approach would be the best option. A hash table would be an alternative, but pointer hashing has some risks.

[eddelbuettel]

We tried, it's all documented in this thread. And redoing this is not an entirely simple fix either.

If used in a normal manner it still works as it has for the previous dozen years, and by now 1950 packages on CRAN.

[clauswilke]

Yeah, I think this is a particularly unusual use case, with gridtext making some poor choices, Rcpp making some poor choices, and @bhive01 wanting to generate thousands of plots in a single RMarkdown chunk. I think it's fine if in some weird corner cases performance is poor. Eventually, I plan to rewrite gridtext completely, since I've learned a lot from my first implementation, and hopefully this issue will go away then.

[eddelbuettel]

That was my read from your code too. There wasn't yet an obvious way to help---but a simpler fix may just be a simple stack for strings we could do 'outside' of the core Rcpp operations relying on simpler and well-tested STL semantics. This really is a corner of a corner case, and I am not quite willing to rock the general stability for this. Those ideas and rough sketches are all good, as are tests so _if_ someone has time _and_ this itch to scratch I will help with solid testing. But sadly I am not going to have time for rewrite of this myself.

[ltierney]

We tried, it's all documented in this thread. And redoing this is not an entirely simple fix either.

If used in a normal manner it still works as it has for the previous dozen years, and by now 1950 packages on CRAN.

Basically my take as well for the R side, but we both know it only takes one case from someone with a large twitter following ... :-)

[bhive01]

Well thanks all for your help, especially to Luke for getting me going again with R-devel patch. Better to be running and slower than not running at all. Hopefully, improvements down the line will resolve this issue entirely.

[clauswilke]

I'll also point out that I wrote my code as weirdly as I did because otherwise there were encoding issues on Windows. If at some point in the future we can use utf-8 on Windows then C++ code relying on extensive string manipulations can simply use native C++ strings and then many of these issues will go away.

[Enchufa2]

Now, is there by chance a truly _minimally_ reproducible example that would not involve the ggplot2 stack, tibbles, and a number of other packages (including one not on CRAN) ?

Here's a simple example:

#include <Rcpp.h>
using namespace Rcpp;

class SimpleClass {
public:
  IntegerVector x;
  SimpleClass(IntegerVector x) : x(x) {}
};

typedef std::vector<SimpleClass> SimpleClassVec;

// [[Rcpp::export]]
SEXP new_simple_class(IntegerVector x) {
  XPtr<SimpleClassVec> ptr(new SimpleClassVec());
  for (int i = 0; i < 1e5; i++) {
    ptr->push_back(SimpleClass(x));
  }
  return ptr;
}

/*** R
system.time(for (i in 1:100) {
  message("Iteration: ", i)
  new_simple_class(1:1e5)
})
*/

In my computer, this stalls at iteration 2-3, and I have to kill the R session. The babsim.hospital package uses simmer to run thousands of simulations and is hitting this issue consistently (cc @olafmersmann).

The numbers in my example above are deliberately exaggerated to hit the issue soon, but, comparatively, this

#include "cpp11.hpp"
#include <vector>
using namespace cpp11;

class SimpleClass {
public:
  integers x;
  SimpleClass(integers x) : x(x) {}
};

typedef std::vector<SimpleClass> SimpleClassVec;

[[cpp11::register]]
SEXP new_simple_class(integers x) {
  external_pointer<SimpleClassVec> ptr(new SimpleClassVec());
  for (int i = 0; i < 1e5; i++) {
    ptr->push_back(SimpleClass(x));
  }
  return ptr;
}

saved as test_cpp11.cpp and then running

cpp11::cpp_source("test_cpp11.cpp")
system.time(for (i in 1:100) {
  message("Iteration: ", i)
  new_simple_class(1:1e5)
})

takes just 7 seconds in my computer.

EDIT: I see the example is similar to https://github.com/RcppCore/Rcpp/issues/1081#issuecomment-629834788 (sorry, long thread :) ).

[eddelbuettel]

For completeness, with the PR #1033 we'll likely merge tomorrow they both take the same time (where I took the message printing out and simplified the outer loop):

> Rcpp::sourceCpp("/tmp/exRcpp.cpp")

> system.time(replicate(100, new_simple_class_rcpp(1:1e5)))
   user  system elapsed 
  2.639   0.012   2.651 
> 
> cpp11::cpp_source("/tmp/exCpp11.cpp")
> system.time(replicate(100, new_simple_class_cpp11(1:1e5)))
   user  system elapsed 
  2.711   0.064   2.774 
> 

[eddelbuettel]

This is now taken care of in Rcpp 1.0.6.2 via https://github.com/RcppCore/Rcpp/pull/1133. Big thanks to everybody who helped with this, it is a very nice fix.