You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Oh but the get_data() call is particularly slow when called from RStudio. i.e. if you run this in a reprex in a separate session it is very fast, but interactively in RStudio it is slow:
>bench::mark(cli:::rstudio$.internal$get_data())
# A tibble: 1 × 13expressionminmedian`itr/sec`mem_alloc`gc/sec`n_itrn_gctotal_timeresult<bch:expr><bch><bch:><dbl><bch:byt><dbl><int><dbl><bch:tm><list>1cli:::rstudio$.in… 787µs809µs1161.41.1KB17.35388463ms<namedlist># ℹ 3 more variables: memory <list>, time <list>, gc <list>
800us * 100 = 800ms which roughly aligns with my profvis result
It looks like most of the time is in the rstudioapi function calls
>bench::mark(asNamespace("rstudioapi")$isAvailable())
# A tibble: 1 × 13expressionminmedian`itr/sec`mem_alloc`gc/sec`n_itrn_gctotal_timeresultmemory<bch:expr><bch:><bch:><dbl><bch:byt><dbl><int><dbl><bch:tm><list><list>1"asNamespac… 1.11µs 1.31µs 700181. 0B 70.0 9999 1 14.3ms <lgl> <Rprofmem># ℹ 2 more variables: time <list>, gc <list>> bench::mark(asNamespace("rstudioapi")$getVersion())# A tibble: 1 × 13 expression min median `itr/sec` mem_alloc `gc/sec` n_itr n_gc total_time result <bch:expr> <bch> <bch:> <dbl> <bch:byt> <dbl> <int> <dbl> <bch:tm> <list> 1 "asNamespace(\"rstu… 384µs 403µs 2414. 20.6KB 17.2 1125 8 466ms <pckg_vrs># ℹ 3 more variables: memory <list>, time <list>, gc <list>> bench::mark(asNamespace("rstudioapi")$versionInfo()$mode)# A tibble: 1 × 13 expression min median `itr/sec` mem_alloc `gc/sec` n_itr n_gc total_time result memory <bch:expr> <bch> <bch:> <dbl> <bch:byt> <dbl> <int> <dbl> <bch:tm> <list> <list> 1 "asNamespace… 382µs396µs2460.20.6KB17.211478466ms<chr><Rprofmem># ℹ 2 more variables: time <list>, gc <list>
One step cli could take is to just make one call to versionInfo(), because that has the version in it as versionInfo()$version, so there is no need to also call $getVersion().
@DavisVaughan Why not cache the number of colors, so rstudio$detect() is only called once and you don't need to wait for any other packages to get updated?
You would cache in the function that make a lot of calls downstream. E.g. if one function calls cli many times then there. If another function calls the first function many times, then there.
If there is such a function, and this is not about speeding up a single call...
I think we can get a big improvement here from switching from expr_deparse() to deparse(), because then we avoid the colour checking path all together.
There was a ~6x performance improvement in #172, by removing the most obvious bottleneck. We'll make it faster in the future by doing more work lazily, which is tracked in #176.
Activity
DavisVaughan commentedon Jul 12, 2023
Not really sure the
readCitationFilepart is "real" here. signaling is definitely slow though, and has been a reoccurring issueDavisVaughan commentedon Jul 12, 2023
See also r-lib/rlang#1609 for
.rlang_cli_compat()being slowDavisVaughan commentedon Jul 12, 2023
I think I've looked at
isatty()before too and I couldn't ever figure out why it was slow. It is so fast on its ownCreated on 2023-07-12 with reprex v2.0.2
DavisVaughan commentedon Jul 12, 2023
Oh but the
get_data()call is particularly slow when called from RStudio. i.e. if you run this in a reprex in a separate session it is very fast, but interactively in RStudio it is slow:800us * 100 = 800ms which roughly aligns with my profvis result
It looks like most of the time is in the rstudioapi function calls
CC @gaborcsardi
DavisVaughan commentedon Jul 12, 2023
Another option is to make
rlang:::has_ansi()cache its result so it doesn't have to keep calling cliDavisVaughan commentedon Jul 12, 2023
It looks like rstudioapi's
versionInfo()is actually the one callingreadCitationFile()!This is used by cli when getting the version number and the mode
https://github.com/r-lib/cli/blob/7eb56ebc4c315ecfef74148881b15a522c8946bf/R/aab-rstudio-detect.R#L35-L36
One step cli could take is to just make one call to
versionInfo(), because that has the version in it asversionInfo()$version, so there is no need to also call$getVersion().But I think a much better solution would be:
rstudioapi::getVersion()not rely onrstudioapi::versionInfo(), so it doesn't pay the cost of the citation file (getVersion()is very slow due toversionInfo()rstudio/rstudioapi#279)rstudioapi::getMode()which just returns the$modeofversionInfo()(Feature request:getMode()as a fast shortcut forversionInfo()$moderstudio/rstudioapi#280)Then have cli switch to using
getMode()DavisVaughan commentedon Jul 12, 2023
I've broken this down to 4 individual fixes
rstudioapi::getVersion()can easily be much fastergetVersion()is very slow due toversionInfo()rstudio/rstudioapi#279rstudioapi::getMode()should exist Feature request:getMode()as a fast shortcut forversionInfo()$moderstudio/rstudioapi#280getMode()overversionInfo()$modeEventually userstudioapi::getMode()once it is available cli#621packageVersion()calls Cache rlang version in.rlang_cli_compat()? rlang#1609gaborcsardi commentedon Jul 12, 2023
@DavisVaughan Why not cache the number of colors, so
rstudio$detect()is only called once and you don't need to wait for any other packages to get updated?DavisVaughan commentedon Jul 12, 2023
Yea I quickly mentioned that here #167 (comment). I do think that would be a good quick solution
It won't be dynamic to any options like
"crayon.enabled"anymore, but maybe that is ok, not suregaborcsardi commentedon Jul 12, 2023
I an not sure what you mean.
DavisVaughan commentedon Jul 12, 2023
has_ansi(), it reportsTRUEbecausecli::num_ansi_colors() > 1isTRUEoptions(crayon.enabled = FALSE)has_ansi()again. It's cached so it reportsTRUEagain even though in theory if it wasn't cached it would reportFALSEi.e. here is what happens today without caching, which would no longer be true with caching
I just don't know how important that is for the places
has_ansi()is usedgaborcsardi commentedon Jul 12, 2023
You would cache in the function that make a lot of calls downstream. E.g. if one function calls cli many times then there. If another function calls the first function many times, then there.
If there is such a function, and this is not about speeding up a single call...
hadley commentedon Nov 1, 2023
I think we can get a big improvement here from switching from
expr_deparse()todeparse(), because then we avoid the colour checking path all together.Improve performance
Improve performance (#172)
hadley commentedon Nov 2, 2023
There was a ~6x performance improvement in #172, by removing the most obvious bottleneck. We'll make it faster in the future by doing more work lazily, which is tracked in #176.