Skip to content

Conversation

DavisVaughan
Copy link
Member

The 3 commits here are self contained. Look at them individually to easily understand this PR.

The goal of this PR is to improve performance of deprecate_soft() and friends. We've been plagued by poor performance here for years, and it really hurts when used repeatedly in a group_by() + mutate() where the function used in mutate() is deprecated or has a deprecated argument.

I've been able to make it ~3x faster by:

  • Swapping glue_data() for sprintf(), at the cost of ergonomics. But this is the biggest win, so I think it is worth it.
  • Swapping arg_match() for arg_match0()
  • Avoiding -.POSIXct and difftime operations when computing the 8 hour time diff, which was doing the wrong thing anyways!!
cross::bench_branches(\() {
  library(lifecycle)
  
  # trigger the 8 hour warning once
  deprecate_soft("1.1.0", I("my thing"), details = "for this reason")

  bench::mark(
    deprecate_soft("1.1.0", I("my thing"), details = "for this reason")
  )
})
#> # A tibble: 2 × 7
#>   branch               expression                    min  median `itr/sec` mem_alloc `gc/sec`
#>   <chr>                <bch:expr>                <bch:t> <bch:t>     <dbl> <bch:byt>    <dbl>
#> 1 fix/deprecation-time "deprecate_soft(\"1.1.0\…  73.1µs  75.3µs    12802.    8.56KB     168.
#> 2 main                 "deprecate_soft(\"1.1.0\… 224.8µs 231.5µs     4229.    8.56KB     176.

I got here by analyzing this profvis output

profvis::profvis(for (i in 1:100000) lifecycle::deprecate_soft("1.5.0", I("a thing"), details = "another thing"))
Screenshot 2025-10-03 at 10 32 46 AM

(expect_condition(signal_stage("superseded", "foo(bar)")))
Output
<lifecycle_stage: foo(arg) is superseded>
<lifecycle_stage: foo(bar) is superseded>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually had a bug here where ({arg}) wasn't being interpolated

msg <- glue::glue_data(what, "{fn}() is {stage}")
msg <- sprintf("%s() is %s", what$fn, stage)
} else {
msg <- glue::glue_data(what, "{fn}(arg) is {stage}")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(arg) here should have been ({arg})

Comment on lines +216 to +218
# More than 8 hours
env_poke(deprecation_env, "test", Sys.time() - 9 * 60 * 60)
expect_false(needs_warning("test"))
expect_true(needs_warning("test"))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This incorrect test started "failing" all of a sudden after I started unclass()ing the POSIXct, which is how I found the bug

@DavisVaughan DavisVaughan merged commit 0def2a6 into main Oct 3, 2025
11 of 12 checks passed
@DavisVaughan DavisVaughan deleted the fix/deprecation-time branch October 3, 2025 14:46
@tanho63
Copy link

tanho63 commented Oct 3, 2025

@DavisVaughan sorry for gh stalking, but very curious: what is this {cross} package?

@DavisVaughan
Copy link
Member Author

https://github.com/DavisVaughan/cross

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.

3 participants