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

Let instrument recurse into environments #15

Closed
mllg opened this issue Feb 27, 2017 · 12 comments
Closed

Let instrument recurse into environments #15

mllg opened this issue Feb 27, 2017 · 12 comments

Comments

@mllg
Copy link
Member

mllg commented Feb 27, 2017

Currently the package only instruments functions which are defined at top level of a package namespace. But sometimes additional functions are stored inside environments which then do not get instrumented. This is also the case for R6 classes which are stored as environment in the namespace.

@gaborcsardi
Copy link
Member

👍

Btw. I need to rethink debugme soon, because it seems to conflict with R-devel (https://cran.rstudio.com/web/checks/check_results_debugme.html), I suspect that this is because of the JIT being enabled in R-devel...

@gaborcsardi
Copy link
Member

Recursing into environments needs to be done with care, because as soon as you have reference semantics, you might have circular references and circular data structures. So we need to keep track of which environments we are already instrumenting. Probably by using the address of the environment.

@gaborcsardi
Copy link
Member

For R6 to work, I also need to go inside lists. Hopefully this won't cause any problems.

@mllg
Copy link
Member Author

mllg commented Oct 23, 2017

Thanks, this seems to work for batchtools.

However, I noticed that the instrumentation does not work properly with inheritance. Here is a simple example using https://github.com/mllg/bugme:

bar = Bar$new(x = 12)
print(bar$getxsquare)

getxsquare() gets instrumented twice.

@gaborcsardi
Copy link
Member

gaborcsardi commented Oct 23, 2017

Oh, yeah, of course. I will somehow mark the already instrumented functions to avoid instrumenting them again.

@gaborcsardi gaborcsardi reopened this Oct 23, 2017
@mllg
Copy link
Member Author

mllg commented Oct 23, 2017

You could port data.table::address() to debugme which gives you addresses for functions and environments.

@gaborcsardi
Copy link
Member

That requires C code, unfortunately, so I would avoid it if possible.

@gaborcsardi
Copy link
Member

Unless I do some .Internal hack.....

@mllg
Copy link
Member Author

mllg commented Oct 23, 2017

5 lines of C code vs obscuring a call to .Internal() and violating CRAN policies? 😕

SEXP address(SEXP x) {
    char buffer[32];
    snprintf(buffer, 32, "%p", (void *)x);
    return(mkString(buffer));
}

What's wrong with C?

@gaborcsardi
Copy link
Member

It makes debugme heavier, as you either need to rely on CRAN binaries or have a compiler.

@gaborcsardi
Copy link
Member

Actually, I can also just change the debug string, so that the second run does not pick it up.

@mllg
Copy link
Member Author

mllg commented Oct 23, 2017

Actually, I can also just change the debug string, so that the second run does not pick it up.

👍

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

No branches or pull requests

2 participants