rstudio / htmltools

Tools for HTML generation and output
https://rstudio.github.io/htmltools/
215 stars 69 forks source link

Relax the required version of rlang #278

Closed cpsievert closed 3 years ago

cpsievert commented 3 years ago

rlang::obj_address() won't be released until at least August

wch commented 3 years ago

Have you tested the performance of this? IIRC, the reason we used obj_address() was to be fast, but in this new version, getFromNamespace() is likely to be slow.

cpsievert commented 3 years ago

I don't have a good benchmark in mind, but even if it is slow, would it be worth delaying release?

schloerke commented 3 years ago

We can set the local variable to NULL. Then in .onLoad, we can call get from namespace once and set the value with a <<-

This allows for rlang to update after htmltools is installed

wch commented 3 years ago

I wouldn't delay the release for it, but there are other ways of getting the function that are faster. For example, it could use .getNamespace(). Or it could fetch sexp_address the first time obj_address is called, and cache it. That's actually probably the best way to go.

obj_address_cached <- local({
  sexp_address <- NULL

  function(x) {
    if (is.null(sexp_address)) {
      sexp_address <<- getFromNamespace("sexp_address", "rlang")
    }
    sexp_address(x)
  }  
})

x <- 1:10

microbenchmark::microbenchmark(
  getFromNamespace("sexp_address", "rlang")(x),
  .getNamespace("rlang")$sexp_address(x),
  obj_address_cached(x)
)
#> Unit: microseconds
#>                                          expr   min     lq    mean median     uq    max neval
#>  getFromNamespace("sexp_address", "rlang")(x) 4.343 4.4815 4.87278 4.5785 4.7365 24.332   100
#>        .getNamespace("rlang")$sexp_address(x) 1.259 1.3100 1.43425 1.3875 1.4650  3.989   100
#>                         obj_address_cached(x) 1.201 1.2575 1.43651 1.3225 1.4070 11.206   100

One thing to be careful of with the local() method: if the function is called at build time, then it will cache the function then, which is bad. It's probably safer to fetch the function in .onLoad().