r-lib / rlang

Low-level API for programming with R
https://rlang.r-lib.org
Other
500 stars 137 forks source link

Can we speed up `env_get()` and `env_get_list()`? #1582

Open DavisVaughan opened 1 year ago

DavisVaughan commented 1 year ago

Hopefully by just moving the check_environment() checks to C. I can live with no default being slow, although that does kind of stink.

This is one place where it feels like a full C level check_environment() would be useful for rlang, rather than just having a really fast R level check_environment() call. I feel like most of the time would be spent bouncing between R and C otherwise.

library(rlang)

env_get_raw <- function(env, nm, default) {
  .Call(rlang:::ffi_env_get, env = env, nm = nm, inherit = FALSE, 
        last = empty_env(), closure_env = environment())
}

ns <- ns_env("vctrs")

# Not supplying `default` is SUPER slow
bench::mark(env_get(ns, "vec_proxy"))
#> # A tibble: 1 × 6
#>   expression                    min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>               <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 env_get(ns, "vec_proxy")   12.6µs   13.7µs    60466.        0B     12.1

# Supplying default is much faster, but still much slower than
# get0()
bench::mark(
  rlang = env_get(ns, "vec_proxy", default = NULL),
  rlang_raw = env_get_raw(ns, "vec_proxy", default = NULL),
  base = get0("vec_proxy", envir = ns, inherits = FALSE, ifnotfound = NULL),
  iterations = 100000
)
#> # A tibble: 3 × 6
#>   expression      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 rlang        3.41µs   4.19µs   226543.        0B     22.7
#> 2 rlang_raw    1.27µs   1.49µs   640050.        0B     19.2
#> 3 base          924ns   1.08µs   893374.        0B     35.7

# Most of the time is in 2 calls to `check_environment()`
check_environment <- rlang:::check_environment
bench::mark(check_environment(ns))
#> # A tibble: 1 × 6
#>   expression                 min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>            <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 check_environment(ns)   1.09µs   1.29µs   737735.        0B        0

Created on 2023-02-28 with reprex v2.0.2.9000

lionel- commented 1 year ago

Can we just improve the default-less case or do you need these to be as performant as they can?

lionel- commented 1 year ago

Also is this urgent?

DavisVaughan commented 1 year ago

Not urgent for this release, just something I noticed

I felt like ideally it would be almost as fast as get0(), it just seems like a shame for the checkers to make it 2-3x slower