R devel - Sep 2022 - R_check_class_etc(x, valid) is "slow" when 'valid' contains class(x)

Hi Mikael,

On 8/28/22 01:13, Mikael Jagan wrote:
> R_check_class_etc(x, valid) spends a nontrivial amount of time finding
> an environment 'rho' containing the definition of class(x),
evaluating
> (in R, not C) methods::.classEnv(class(x)).
>
> It then returns the result of R_check_class_and_super(x, valid, rho).
> But R_check_class_and_super() does not use 'rho' at all in the
trivial
> case where class(x) is found in 'valid'.
right, that could be improved. Do you have an example which exhibits the 
problem, have you found this by profiling something?
>
> My feeling is that this can be improved.? I am happy to contribute a 
> patch,
> if it would be considered by R-core.
Both R_check_class_etc and R_check_class_and_super are unfortunately 
exported, the former is used a lot in packages (even though they are not 
mentioned in Writing R Extensions, so actually shouldn't be used in 
packages). Anyway, it would be easier if we could preserve their 
interface and behavior.

Maybe we could support rho==NULL in R_check_class_and_super, the 
environment would be looked up in that case when needed. 
R_check_class_etc would simply only call R_check_class_and_super with 
that argument. I see that R_check_class_and_super uses asChar() on the 
class attribute, while R_check_class_etc does not currently for looking 
for the environment, but I assume doing that in both cases should not 
matter (and it would have to be tested).

So this would be a trivial change, but if you wanted to create a minimal 
patch, I will be happy to have a look.

Best
Tomas

> Mikael
>
> ______________________________________________
> R-devel at r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel

On 2022-09-02 5:30 am, Tomas Kalibera wrote:
> Hi Mikael,
> 
> On 8/28/22 01:13, Mikael Jagan wrote:
>> R_check_class_etc(x, valid) spends a nontrivial amount of time finding
>> an environment 'rho' containing the definition of class(x),
evaluating
>> (in R, not C) methods::.classEnv(class(x)).
>>
>> It then returns the result of R_check_class_and_super(x, valid, rho).
>> But R_check_class_and_super() does not use 'rho' at all in the
trivial
>> case where class(x) is found in 'valid'.
> right, that could be improved. Do you have an example which exhibits the 
> problem, have you found this by profiling something?
I benchmarked .Call("R_etc", x) for x <-
new("dgTMatrix"), after compiling
and loading the following:

```
#include <Rinternals.h>

SEXP R_etc(SEXP x) {
     static const char *valid[] = {"dgCMatrix", "dgRMatrix",
"dgTMatrix", ""};
     return ScalarInteger(R_check_class_etc(x, valid));
}
```

R-devel used ~2 microseconds while my patched version of R used ~0.2
microseconds.  So, not a bottleneck by any means.  On the other hand,
'Matrix' and probably other packages do call R_check_class_etc() quite
often, hence my report.
>>
>> My feeling is that this can be improved.? I am happy to contribute a
patch,
>> if it would be considered by R-core.
> 
> Both R_check_class_etc and R_check_class_and_super are unfortunately
exported,
> the former is used a lot in packages (even though they are not mentioned in
> Writing R Extensions, so actually shouldn't be used in packages).
Anyway, it
> would be easier if we could preserve their interface and behavior.
> 
> Maybe we could support rho==NULL in R_check_class_and_super, the
environment
> would be looked up in that case when needed. R_check_class_etc would simply
only
> call R_check_class_and_super with that argument. I see that 
> R_check_class_and_super uses asChar() on the class attribute, while 
> R_check_class_etc does not currently for looking for the environment, but I
> assume doing that in both cases should not matter (and it would have to be
tested).
> 
> So this would be a trivial change, but if you wanted to create a minimal
patch,
> I will be happy to have a look.
I've implemented essentially what you've described; see attached.

Mikael
> 
> Best
> Tomas
> 
> 
>> Mikael
>>
>> ______________________________________________
>> R-devel at r-project.org mailing list
>> https://stat.ethz.ch/mailman/listinfo/r-devel
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: patch.diff
URL:
<https://stat.ethz.ch/pipermail/r-devel/attachments/20220902/7e17c4be/attachment.ksh>