Skip to content

Lazily evaluate named interpolation variables#83

Closed
egnha wants to merge 6 commits into
tidyverse:masterfrom
egnha:lazy-eval-dots
Closed

Lazily evaluate named interpolation variables#83
egnha wants to merge 6 commits into
tidyverse:masterfrom
egnha:lazy-eval-dots

Conversation

@egnha

@egnha egnha commented Apr 5, 2018

Copy link
Copy Markdown
Contributor

A priori, an interpolation string need not reference every variable supplied as a named dot-argument of glue_data(), so there is no need to evaluate them eagerly. In particular, glue("{x}", x = "blah", y = {Sys.sleep(1); "y"}) ought to be as fast as glue("{x}", x = "blah"), and glue("blah", x = stop("!")) should also yield as_glue("blah").

This PR implements (ordered) lazy evaluation of named interpolation variables.

If this is desirable, a review would be appreciated.
Thanks for your consideration.

@egnha

egnha commented Apr 7, 2018

Copy link
Copy Markdown
Contributor Author

f5c7d14 is a much faster and more concise way to lazily bind interpolation arguments.

Here are the three ways of binding interpolation variables:

# Faster, functionally equivalent version of 'bind_ordered_promises()'
bind_promises <- function(args, parent) {
  promises <- as_promises(args)
  parent.env(promises) <- parent
  promises
}
as_promises <- function(args) {
  eval(call("function", as.pairlist(args), quote(environment())))()
}

# da715b81676d66df5c07abc1df98566292686be0
bind_ordered_promises <- function(args, parent) {
  env_assign <- parent
  for (nm in names(args)) {
    env_eval <- env_assign
    env_assign <- new.env(parent = env_assign)
    eval(bquote(delayedAssign(nm, .(args[[nm]]), env_eval, env_assign)))
  }
  env_assign
}

# Eager evaluation (current master)
assign_args <- function(args, envir) {
  # I've disabled the following orphaned assignment, probably a refactoring oversight
  # res <- vector("list", length(args))
  nms <- names(args)
  for (i in seq_along(args)) {
    assign(nms[[i]], eval(args[[i]], envir), envir = envir)
  }
}

Here's how they stack up:

parent <- environment()
envir <- new.env()
x <- "x"; y <- "y"; z <- "z"
args <- alist(x = x, y = y, z = z)

microbenchmark::microbenchmark(
  bind_promises(args, parent),
  bind_ordered_promises(args, parent),
  assign_args(args, envir),
  times = 1e5
)

#> Unit: microseconds
#>                                 expr    min     lq      mean median     uq       max neval
#>          bind_promises(args, parent)  3.218  4.382  5.882061  4.923  5.393 58631.224 1e+05
#>  bind_ordered_promises(args, parent) 38.783 46.881 54.960408 50.372 53.852 44366.326 1e+05
#>             assign_args(args, envir)  6.192  8.048  9.406942  8.723  9.420  3496.235 1e+05

@egnha

egnha commented Apr 7, 2018

Copy link
Copy Markdown
Contributor Author

d21268e corrects an oversight: lazy scoping should occur from left to right to preserve the existing API. Additional test cases added.

Thus bind_promises() is modified as follows:

bind_promises <- function(args, parent) {
  env <- env_init <- new.env(parent = baseenv())
  for (i in seq_along(args))
    env <- as_promise(.subset(args, i), env)
  parent.env(env_init) <- parent
  env
}

as_promise <- function(arg, env) {
  make_promise <- as.call(c(
    call("function", as.pairlist(arg), quote(environment())),
    arg
  ))
  eval(make_promise, env)
}

The complexity of bind_promises() is O(#variables) for a constant of about 5 microseconds.

parent <- environment()
envir <- new.env()
x <- "x"; y <- "y"; z <- "z"
args <- alist(x = x, y = y, z = z)

microbenchmark::microbenchmark(
  bind_promises(args, parent),
  bind_ordered_promises(args, parent),
  assign_args(args, envir),
  times = 1e5
)

#> Unit: microseconds
#>                                 expr    min     lq      mean median      uq       max neval
#>          bind_promises(args, parent) 11.624 15.196 18.499932 16.453 17.8200 66063.555 1e+05
#>  bind_ordered_promises(args, parent) 38.426 47.695 56.625133 50.986 54.8820 64272.838 1e+05
#>             assign_args(args, envir)  6.196  8.128 10.018293  8.825  9.5590 56623.842 1e+05

@jimhester

Copy link
Copy Markdown
Collaborator

Thanks! I ended up using delayedAssign() slightly differently than your proposal and merged this manually with bcfaf72

@jimhester jimhester closed this Apr 9, 2018
@egnha

egnha commented Apr 9, 2018

Copy link
Copy Markdown
Contributor Author

Great, thanks!

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

Successfully merging this pull request may close these issues.

2 participants