r-lib / rlang

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

~`try_fetch()` seems much slower than the deprecated `with_handlers()`~ #1612

Closed lorenzwalthert closed 1 year ago

lionel- commented 1 year ago

You could have left some explanation in the comment :P

lorenzwalthert commented 1 year ago

I wanted to delete the issue (and tilde does not work for strike through in title apparently) since it seems try_fetch() is much faster according to my local benchmark, and our cloud benchmark was using an old version of {rlang}. 🙃 Here's my local benchmark:

library(rlang)
bench::mark(
  rlang::with_handlers({}, error = function(...) NULL),
  rlang::try_fetch({}, error = function(...) NULL),
)
#> Warning: `with_handlers()` is deprecated as of rlang 1.0.0.
#> ℹ Please use `tryCatch()`, `withCallingHandlers()`, or `try_fetch()`.
#> This warning is displayed once every 8 hours.
#> # A tibble: 2 × 6
#>   expression                                                 min  median itr/s…¹
#>   <bch:expr>                                            <bch:tm> <bch:t>   <dbl>
#> 1 rlang::with_handlers({ }, error = function(...) NULL)   6.37ms  6.47ms    152.
#> 2 rlang::try_fetch({ }, error = function(...) NULL)      10.75µs 11.62µs  84532.
#> # … with 2 more variables: mem_alloc <bch:byt>, `gc/sec` <dbl>, and abbreviated
#> #   variable name ¹​`itr/sec`

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

lorenzwalthert commented 1 year ago

And our {touchstone} benchmark showed big speed penalty: https://github.com/r-lib/styler/pull/1103#issuecomment-1492850139

lionel- commented 1 year ago

yup I would have expected try_fetch() to be much faster than with_handlers(). We're still slower than tryCatch() but unfortunately I don't think we can do better here. We used to, by being a pure withCallingHandlers() wrapper, but we had to reintroduce a tryCatch() to handle stack overflow errors.

lorenzwalthert commented 1 year ago

Things look still odd in our benchmark, since all we did was replacing with_handlers() with try_fetch() and requiring {rlang} > 1.0.0 and it takes 3x the time.