-
Notifications
You must be signed in to change notification settings - Fork 7
Remove interface layer of jaspResults #6
base: master
Are you sure you want to change the base?
Conversation
…free errors, possibly related to R's garbage collection
…ng jaspResults objects
So right now we have 4 layers:
After this PR, we'd have 3 layers:
However, we can also cut the S4 layer, and have:
here, the R6 would contain a "smart" pointer (i.e., Removing the S4 layer might be cleaner, but, it does imply that we need to rewrite all the internals of the R-side. For example, we can no longer do Rcpp::CharacterVector getColNames(Rcpp::XPtr<jaspTable*> ptr)
{
ptr->_colNames;
}
void setColNames(Rcpp::XPtr<jaspTable*> ptr, Rcpp::CharacterVector newNames)
{
ptr->_colNames = newNames;
} on the R side for the developers, nothing changes of course since they only interact with the R6 layer. @JorisGoosen any thoughts? |
This has the problem that the pointer in S4 is copied by the Rcpp::Modules stuff whenever the S4 object is assigned to a variable. Which was the original reason for having that
This would definitely solve the problem alluded to above with the S4 contains ref to
//Probably best not to directly reference private/internal variables like that though.
How else would we do this? Well... If we are going to be manually coding these wrappers then we are recreating the just removed Interface + modules right? So the only way this would be an improvement is if we could generate this, and this pkg does that: https://github.com/richfitz/RcppR6
Indeed |
Yes, the main reason to do this is that it would make things more lightweight. Note that I'm not really sure whether it makes sense to cut out this layer at all, these are just some ideas about how to cut it out, if we want to do so.
That's an option, but there you basically do:
So there are two options.
Rcpp::CharacterVector getColNames(Rcpp::XPtr<jaspTable*> ptr)
{
return ptr->getColNames();
}
void setColNames(Rcpp::XPtr<jaspTable*> ptr, Rcpp::CharacterVector newNames)
{
ptr->setColNames(newNames);
} and on the R side, we write manually jaspTableR <- R6::R6class(
... # stuff
active = list(
colNames = function(x) { if missing(x) getColNames(private$jaspObjectPtr) else setColnames(jaspObjectPtr, x) }
),
# inherited from jaspObjectR
private = list(
jaspObjectPtr = new("externalptr") # default is null pointer
)
) For option 1, the layers become
For option 2, the layers become
Rather than 1, I'd also be happy to stick with the generated S4 layer, as I'm not sure what it would add to replace S4 with R6 (instead of cutting it out). Note that both the generated S4 and R6 also define the wrapper functions for option 2, but they just hide those functions in a class interface. |
It would make adding new stuff a lot easier because it would go from adding two manual codes (one in
Ok, so that doesn't help too much then.
This is basically the same as what the
Maybe we should have a look at https://github.com/r-lib/cpp11 ? If we can somehow call functions on a pointer to an object a bit easier from R it would make it a lot more smooth. |
What might be possible is calling the C++ functions in such a way that we supply the normally invisible |
The difference is that the wrapper function I suggest functions are directly callable from R, whereas the jaspObject_interface-classes store a pointer but still require another C++ wrapper function to be exposed in R. See example 1.2 here where they manually expose a class. I'd intend to generate those wrapper functions (e.g.,
In cpp11, the approach where we use functions with pointers to jaspObjects would translate to something like this: cpp11::strings getColNames(cpp11::external_pointer<jaspTable*> ptr)
{
return ptr->getColNames();
}
void setColNames(cpp11::external_pointer<jaspTable*> ptr, cpp11::strings newNames)
{
ptr->setColNames(newNames);
} cpp11 is a newer C++ to R interface. It's smaller, has faster compilation times, and does more things that we want, for example, automatic UTF-8 conversions for everything passed from R. However, cpp11 does not support modules, so we could only use this if we write the glue code ourselves.
that would be pretty cool, however, doesn't that mean we need to write those functions with SEXP? If that's the case, I think this might introduce more problems than it would solve 😛 |
Ah yes, ok that is nicer.
Well, that would just mean a lot of rewriting and the UTF-8 conversion is (a) already covered now and (b) will be obsolete soon anyway once the
Well, not necessarily with Rcpp because then we could at least use those types (Rcpp::String etc) in the glue right? But this would need to be tested to see whether we can pull that off, if I look at that example you linked it is at least not something they suggest! |
The thing is, if we implement the wrappers functions we can mostly use std/ JSON structures for all jaspObject classes and have the conversion between R and C++ structures be done by the wrappers function. At that point, switching between Rcpp and cpp11 (or at least, trying it out) should be relatively easy (updating the types for generated wrappers + the types for the state).
What they say this:
Whether we use S4 or R6 for the wrapping doesn't really matter I think, that's just about exposing it to users. |
Sure, but that would then also entail rewriting all the code on all jaspObjects that now handle Rcpp classes directly. So while this is good for future support of other languages/cp11 it does add a significant amount of work there. At least, I think.
Yeah these pointers would obviously be inside the R6 not exposed to the user that would be terrible. |
So I was playing a bit with this and it turned out to be not too difficult.
I think the main thing to check is if specializing Rcpp's
standard_delete_finalizer
doesn't introduce a memory leak. I don't think so, but @JorisGoosen probably knows better than me. Perhaps that function should only be specialized in JASP (because we calldestroyAllAllocatedObjects
elsewhere), and in R we can just let the garbage collector do its thing.In addition, I changed properties on jaspInterface with getters and setters that did nothing but forward to the jaspObjects to fields on the jaspObjects, e.g.,
Before we'd ever merge this, I think we should (locally) run all unit tests to see if this doesn't break anything.