Improve perf of cols_nanoplot() by not resolving + use more vctrs

[olivroy]

Use less dplyr in the codebase in favor of vctrs for speed + add a workaround to disable column resolve when it is not necessary to speed up cols_nanoplot()

The profvis profiling data shows the following now:

With the latest commit

Previous commit on the PR

This basically divides in 2 the amount of time spent on resolving columns (-0.5 seconds)

It is important that the intermediate envvar "GT_RESOLVE_LOCALE" should never be set explicitly in tests or anywhere else. It could theoretically be added to every vec_fmt_*() function, but I added it in the nanoplot code since this is where it is used more extensively

Summary

Just a continuation of previous work to improve speed

Related GitHub Issues and PRs

• Follow-up to Improve performance by having boxhead as a data frame. #1896

Checklist

• I understand and agree to the Code of Conduct.
• I have listed any major changes in the NEWS.
• I have added testthat unit tests to tests/testthat for any new functionality.

[rich-iannone]

Everything here is great except for the one concern with the withr use in user-facing code.

[rich-iannone]

This is a great idea but we currently have withr only in Suggests (and no room left in Imports currently). Could this be done safely the old way, using on.exit() to unset the envvar?

[olivroy]

Thanks for the review and the suggestion (made me work a bit more, but the code seems better now that it avoids withr.

I read the source code of withr::local_envvar() more closely and found a way to make it work.

I also included more tests + a documentation fix.

This looks a bit sketchy, but I couldn't find a way other than env vars to pass down information across frames.

[rich-iannone]

Thanks for making the change here (missed it before!).

[rich-iannone]

Thanks for making these changes! Really do appreciate it. Will merge now.

[olivroy]

@rich-iannone, sorry for the many PRs (again), For context, I am developing a shiny app with some tables that contain nanoplots )` and apart from these (now mostly fixed) performance issues, they work very well, so props to you on that great feature!

I'd just like to reiterate that if you have some time, it would be great to release a patch soon. I think the package is at a reasonably good stop point with the extract_body() fix. It would be easier to concentrate on the upcoming bigger chunks for the 0.12.0 release.

I am generally okay with using dev package in prod, but there have been 461 commits to the main branch since release ;)

[rich-iannone]

@olivroy Yeah, totally agree, this needs to be released. I'm really encouraged (i.e., blown away) by all the speedups you made so I'm feeling inspired to submit just one more PR that migrates most of the remaining rlang::match() calls to rlang::match0(). I ran the benchmark (I think you might've shown the same thing before) and the time savings are great:

test_fn_match <- function(value = c("one", "two", "three")) {
  rlang::arg_match(value)
}

test_fn_match_0 <- function(value = c("one", "two", "three")) {  
  rlang::arg_match0(value, c("one", "two", "three"))
}

test_fn_match_0_extern <- function(value = c("one", "two", "three")) {  
  check_vals <- c("one", "two", "three")
  rlang::arg_match0(value, check_vals)
}

res <- 
  microbenchmark::microbenchmark(
    test_fn_match(value = "two"),
    test_fn_match_0(value = "two"),
    test_fn_match_0_extern(value = "two"),
    times=200000L
  )

print(res)

Unit: nanoseconds
                                  expr   min    lq       mean median    uq       max neval
          test_fn_match(value = "two") 15662 17589 20190.6366  18163 18737 107669116 2e+05
        test_fn_match_0(value = "two")   615   779   939.4918    861   943   3175983 2e+05
 test_fn_match_0_extern(value = "two")   656   820   990.3896    902   984   3369790 2e+05

I should be able to do this pretty quickly.

[olivroy]

Great idea! Yeah, they are quite different. I only focused on the ones that are called very often.

1. You can't swap it when multiple = TRUE (i.e. arg_match0() doesn't have a multiple argument.
2. For those with many choices, such as the context argument of vec_fmt_*(), maybe using a global vector / or a general check_context() function for the context argument would be good?

Most of the performance improvements I made was simply to avoid calculating things unnecessarily.

The performance of arg_match() is only a problem if it gets called 1000s of times.

15662ns is still only 0.0001 seconds (using arg_match0() instead only has a significant impact if it gets called 6000 times (about 1 sec).

I think we have improved significantly over the past year. General rendering is pretty fast now

[rich-iannone]

Just for the sake of completionism (and because it's so easy) I'll make these proposed changes. Thanks for the tips, too! The hardcoding/duplication is a drag but the performance improvements are worth it. Will definitely use global vector in some cases where there is a lot of repetition. Plus I won't touch the cases where multiple choices is valid.