r-lib / R6

Encapsulated object-oriented programming for R
https://R6.r-lib.org
Other
409 stars 56 forks source link

Avoid using `$` on environments that may not be R6 objects #274

Closed zeehio closed 1 year ago

zeehio commented 1 year ago

Avoids using $ on environments we don't control, because unexpected things may happen (such as #253).

zeehio commented 1 year ago

Thanks @IndrajeetPatil for your review.

While reviewing this I realized this solution has a significant performance impact so I improved it using get0(), introduced in R-3.2.

I decided to benchmark the cost of this patch. TLDR: the patch as it is now has no performance impact

Since the size of the environment has an impact, I tested results with two environment sizes:

Since the performance may be different, due to method dispatch, depending on whether the environment has a class attribute or not I checked:

create_env_with_items <- function(nitems = 1000, class. = NULL) {
  e <- new.env(parent = emptyenv())
  for (i in seq_len(nitems)) {
    e[[paste0("A", as.character(i))]] <- runif(1)
  }
  if (!is.null(class.)) {
    class(e) <- class.
  }
  e
}

env_with_class_1item <- create_env_with_items(1, "bar")
env_without_class_1item <- create_env_with_items(1)

env_with_class_100000item <- create_env_with_items(100000, "bar2")
env_without_class_100000item <- create_env_with_items(100000)

orig_buggy <- function(value) {
  is.environment(value) && !is.null(value$`.__enclos_env__`)
}

with_get0 <- function(value) {
  is.environment(value) &&
    !is.null(get0(".__enclos_env__", envir = value, inherits = FALSE))
}

microbenchmark::microbenchmark(
  origbuggy_notclassed_1it = orig_buggy(env_without_class_1item),
  origbuggy_classed_1it = orig_buggy(env_with_class_1item),
  withget0_notclassed_1it = with_get0(env_without_class_1item),
  withget0_classed_1it = with_get0(env_with_class_1item),
  origbuggy_notclassed_100kit = orig_buggy(env_without_class_100000item),
  origbuggy_classed_100kit = orig_buggy(env_with_class_100000item),
  withget0_notclassed_100kit = with_get0(env_without_class_100000item),
  withget0_classed_100kit = with_get0(env_with_class_100000item)
)
#> Unit: nanoseconds
#>                         expr  min     lq     mean median     uq     max neval
#>     origbuggy_notclassed_1it  652  692.0 23898.10  710.5  738.0 2314056   100
#>        origbuggy_classed_1it 1422 1539.5  1635.23 1611.5 1660.5    3344   100
#>      withget0_notclassed_1it 1834 1937.5  2230.37 2002.5 2080.5   16372   100
#>         withget0_classed_1it 1866 1989.0  2411.46 2052.5 2109.0   36883   100
#>  origbuggy_notclassed_100kit  653  710.0   888.63  739.0  764.0   14474   100
#>     origbuggy_classed_100kit 1383 1527.0  1621.91 1579.0 1646.5    5164   100
#>   withget0_notclassed_100kit 1859 1965.5  2313.70 2047.5 2121.0   27013   100
#>      withget0_classed_100kit 1917 1997.0 31067.35 2063.0 2130.0 2894422   100

Created on 2023-04-17 with reprex v2.0.2

zeehio commented 1 year ago

Thanks for the review @wch. I took care of all of your suggestions without any issues