Dark hackery

[DavisVaughan]

@hadley if you combine id into the mix which avoids lifecycle_message() generation in the main path (see #197), then pretty much all of the remaining overhead moves into cnd_signal() and signalCondition() not being as lazy as we'd like. So this dark hackery starts to look more interesting...

cross::bench_branches(\() {
  library(lifecycle)
  
  # trigger the per session warning once
  deprecate_soft("1.1.0", I("my thing"), details = "for this", id = "foo")

  bench::mark(
    deprecate_soft("1.1.0", I("my thing"), details = "for this", id = "foo"),
    iterations = 100000
  )
})
#> # A tibble: 2 × 7
#>   branch               expression                      min median `itr/sec` mem_alloc `gc/sec`
#>   <chr>                <bch:expr>                   <bch:> <bch:>     <dbl> <bch:byt>    <dbl>
#> 1 feature/dark-hackery "deprecate_soft(\"1.1.0\", … 13.2µs 13.9µs    68580.    8.56KB    102. 
#> 2 main                 "deprecate_soft(\"1.1.0\", … 31.7µs 33.2µs    28957.    8.56KB     66.5

I'm still not entirely sure how safe this is, because I think there is a path where "dummy" would get shown to the user as some error message if they somehow promote our lifecycle_stage condition to an error?

https://github.com/wch/r-source/blob/f200c30b1a20dfa9394d7facff616e9cb2a42c6d/src/main/errors.c#L1904-L1909

[DavisVaughan]

Also note that cnd_signal() is not 1:1 with signalCondition() because cnd_signal() also adds a non-zero overhead call to withRestarts() to add a rlang_muffle restart. Do we need that? If not, then at the very least we could just call signalCondition() directly to avoid that overhead from cnd_signal() even if we don't do this hackery.

---

I have tracked the rlang_muffle thing back to this commit in rlang
r-lib/rlang@2c1bd29

Ah okay, so "bare conditions" signaled with signal() or cnd_signal() are mufflable by rlang::cnd_muffle() due to the addition of this rlang_muffle restart, while signalCondition() bare conditions are not. In other words this rlang_muffle restart helps bare conditions be more like warning and message conditions, which are always mufflable due to having muffleWarning and muffleMessage restarts available.

On one hand that feels important to keep around, but on the other hand when would you ever want to muffle this kind of signal which doesn't have any side effects? (unlike messages and warnings that print something)

Here's what just switching cnd_signal() to signalCondition() would look like, which we can do without any hackery. We lose the ability to muffle a lifecycle_signal condition because there won't be a rlang_muffle restart around anymore, but maybe that is totally fine.

cross::bench_branches(\() {
  library(lifecycle)
  
  # trigger the per session warning once
  deprecate_soft("1.1.0", I("my thing"), details = "for this", id = "foo")

  bench::mark(
    deprecate_soft("1.1.0", I("my thing"), details = "for this", id = "foo"),
    iterations = 100000
  )
})
#> # A tibble: 2 × 7
#>   branch                        expression                                   min median `itr/sec` mem_alloc `gc/sec`
#>   <chr>                         <bch:expr>                                <bch:> <bch:>     <dbl> <bch:byt>    <dbl>
#> 1 feature/bare-signal-condition "deprecate_soft(\"1.1.0\", I(\"my thing\… 25.3µs 26.6µs    36226.    8.56KB     71.9
#> 2 main                          "deprecate_soft(\"1.1.0\", I(\"my thing\… 31.7µs   33µs    29478.    8.56KB     67.7

[DavisVaughan]

I've opened #199 if we want to do this

[lionel-]

when would you ever want to muffle this kind of signal which doesn't have any side effects?

Other handlers might have side effects. We have never really used that restart AFAIK, so we can remove it: r-lib/rlang#1836

[lionel-]

Just voicing that I think this is inappropriate to do.