Early exit for empty index in vec_assign()

[mgirlich]

Closes #1408.

For example this is relevant for coalesce() like uses, e.g. vec_assign(x, is.na(x), 1L).

I only added this optimisation for vec_assign_opts() and not for other functions in slice-assign.h e.g. vec_proxy_assign_opts(). I only saw vec_proxy_assign_opts() being called in unchop() and vec_c() but it don't think one would encounter an empty index there.

Do you want an explicit test that vec_assign(x, integer(), -1L) works? It is currently tested "indirectly" via vec_slice(x, FALSE) <- 2L.

library(rlang)
library(vctrs)

n <- 10e6
x <- 0L + 1:n
idx <- vec_rep(FALSE, n)

bench::mark(
  false_n = vec_assign(x, idx, -1L),
  false = vec_assign(x, FALSE, -1L),
  empty = vec_assign(x, integer(), -1L),
  check = FALSE,
  min_iterations = 20
)

# Before
#> # A tibble: 3 × 6
#>   expression      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 false_n     13.98ms  14.58ms      62.8    38.1MB     68.1
#> 2 false        5.07ms   5.49ms     179.     38.1MB    172. 
#> 3 empty        5.03ms   5.59ms     177.     38.1MB    171.

# After
#> # A tibble: 3 × 6
#>   expression      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 false_n      8.37ms   9.01ms      109.     3.5KB      0  
#> 2 false        1.74µs   1.86µs   371996.        0B     37.2
#> 3 empty        2.11µs   2.29µs   341040.        0B      0

^{Created on 2022-07-15 by the reprex package (v2.0.1)}

[lionel-]

Do you want an explicit test that vec_assign(x, integer(), -1L) works?

Yes please, it never hurts to add one more test :-)

[DavisVaughan]

I'm actually tempted to put this optimization deep within the assignment macros, like ASSIGN_INDEX(), ASSIGN_COMPACT(), the corresponding BARRIER macros, and the corresponding "shaped" ones for arrays. Otherwise I think it is very hard to get the placement of the optimization right, because we aren't always exactly sure what index is (is it compact, or not?) and we have all kinds of other checks on x/proxy/value that we want to ensure are run before we decide to early exit.

Basically I think the early exit should be done as close to the generation of the copy as possible (i.e. the vec_clone_referenced() call)

[DavisVaughan]

I think the cast and recycle checks need to run before we can think about early exiting

[lionel-]

Closing for the reasons explained in #1636 (comment)

[hadley]

Reopening, because I'd like to reconsider in light of tidyverse/tidyr#1392 — the performance gains seem meaningful, and it seems much more reasonable to handle this in vctrs rather than having to wrap every use of vec_assign().