ALTREP list performance fix: Never clone in vec_clone_referenced() when owned

[DavisVaughan]

In #1151, I reverted us to a simpler ownership model.

However, I added in an idea where we unconditionally shallow duplicate ALTREP objects before we try to assign to them. I left a few comments about this in the description and in the code

When we own the object, we only ever attempt to duplicate it if it is ALTREP. If vec_init() ever creates ALTREP objects in the future (#837), this will be required. When doing assignment, we have to duplicate ALTREP objects before dereferencing even if we own them, because we need access to the actual data that it is representing, not the ALTREP object's internals.

// If `x` is ALTREP, we must unconditionally clone it before dereferencing,
// otherwise we get a pointer into the ALTREP internals rather than into the
// object it truly represents.

// - If `owned` is `VCTRS_OWNED_true`, the `proxy` is typically not duplicated.
//   However, if it is an ALTREP object, it is duplicated because we need to be
//   able to assign into the object it represents, not the ALTREP SEXP itself.
// - If `owned` is `VCTRS_OWNED_false`, the `proxy` is only

I now believe I was mistaken about how ALTREP worked. In particular, the idea that we need to duplicate ALTREP objects before dereferencing them (with something like INTEGER()) is just wrong:

• Duplicating an ALTREP object with Rf_shallow_duplicate() doesn't guarantee that you get a non-ALTREP object. In particular, shallow duplication is actually a nice way to generate wrapper ALTREP objects. I also think vroom ALTREP objects can be duplicated and return another ALTREP object.
• Dereferencing an ALTREP object with anything that goes through DATAPTR() should be completely safe. It forces ALTREP materialization but is then required to give you a pointer to "actual" memory representing that type. And I'm not worried about the "force materialization" idea here being expensive - since we "own" the data in the path where we would force materialization at assignment time (i.e. from vec_init()), the only kind of ALTREP objects we should encounter are cheap-to-materialize wrapper ALTREP objects like you get from proxying the init-ed object, like with vec_proxy.vctrs_list_of() calling unclass() to drop off the class attribute.

---

So that is a description of why I don't think we need to duplicate ALTREP objects unconditionally, but why has this come up all of a sudden? In R-devel (4.4.0), ALTREP list vectors were added. That has caused this memory related test to start failing:

vctrs/tests/testthat/test-c.R

Lines 578 to 587 in 8bbd8c4

	"list-ofs (#1496)"
	make_list_of <- function(n) {
	df <- tibble::tibble(
	x = new_list_of(vec_chop(1:n), ptype = integer())
	)
	vec_chop(df)
	}
	with_memory_prof(list_unchop(make_list_of(1e3)))
	with_memory_prof(list_unchop(make_list_of(2e3)))
	with_memory_prof(list_unchop(make_list_of(4e3)))

With a reprex of:

library(vctrs)

make_list_of <- function(n) {
  df <- tibble::tibble(
    x = new_list_of(vec_chop(1:n), ptype = integer())
  )
  vec_chop(df)
}

xx <- make_list_of(4e3)
profmem::profmem(list_unchop(xx))

Indeed this runs much slower and allocates MUCH more memory in R-devel. It is a little complicated due to the tibble in the mix, but I can try to explain it. make_list_of() allocates a list of very small tibbles containing list-ofs:

make_list_of(2)
#> [[1]]
#> # A tibble: 1 × 1
#>             x
#>   <list<int>>
#> 1         [1]
#> 
#> [[2]]
#> # A tibble: 1 × 1
#>             x
#>   <list<int>>
#> 1         [1]

Note that 4e3 == 4,000 so with_memory_prof(list_unchop(make_list_of(4e3))) allocates a list of 4000 of these tibbles and then binds them together using list_unchop(). To do this:

• vec_init() allocates the result data frame containing the list-of column and recursively proxies it (so the list-of column is proxied too)
• vec_proxy.vctrs_list_of() is called, which only does unclass(x), but this makes an ALTREP wrapper version of the list-of column in R-devel (as long as the object has "enough" elements, and 4k is enough).
• We start the assignment loop in vec_c(). On the first iteration we dig into df_assign(), extract the proxy ALTREP list-of column we are going to assign to, and then go into vec_proxy_assign_opts() to assign into it
• In vec_proxy_assign_opts(<proxy-altrep-list-of>, 0, <first-list-of>) we try to assign the first list-of element into the proxy by going through list_assign(). But this guards the assignment with vec_clone_referenced(proxy, owned). Since proxy is an ALTREP list-of, this GETS SHALLOW DUPLICATED and a fresh ALTREP wrapper is made, and then at SET_VECTOR_ELT() time a full duplication is made so we can assign to it, even though we fully own it!
• Because the new proxy returned from that iteration of vec_proxy_assign_opts() is also an ALTREP wrapper, we go through this on every iteration, making 4k duplicates of proxy in total.

In this PR, we change this cycle by not doing a shallow duplication in vec_clone_referenced() because we own the proxy ALTREP list-of. When we call SET_VECTOR_ELT() the first time, this may do 1 full duplication on the first iteration (not sure) to materialize the wrapper ALTREP's data, but after that it is treated like a non-ALTREP object for every other iteration because data2 is filled out and we don't get this explosive duplication.

library(vctrs)

make_list_of <- function(n) {
  df <- tibble::tibble(
    x = new_list_of(vec_chop(1:n), ptype = integer())
  )
  vec_chop(df)
}

xx <- make_list_of(4e3)

y <- list_unchop(xx)
y <- list_unchop(xx)
y <- list_unchop(xx)

# CRAN vctrs - R 4.3.1
bench::mark(list_unchop(xx), iterations = 20)
#> # A tibble: 1 × 6
#>   expression           min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>      <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 list_unchop(xx)    145ms    149ms      6.65     141KB     45.6

# CRAN vctrs - R 4.4.0 (oh no!)
bench::mark(list_unchop(xx), iterations = 20)
#> # A tibble: 1 × 6
#>   expression           min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>      <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 list_unchop(xx)    220ms    246ms      3.97     122MB     33.5

# This PR - R 4.4.0
bench::mark(list_unchop(xx), iterations = 20)
#> # A tibble: 1 × 6
#>   expression           min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>      <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 list_unchop(xx)    146ms    148ms      6.65     110KB     49.6

[lionel-]

Sounds plausible! I can spend some time to double check the reasoning more thoroughly if you think that'd be helpful here.

[DavisVaughan]

@lionel- I'm going to try and run cloud revdeps with r-devel, if that looks good then I don't think I need a second review (unless you want to), but thanks!

[DavisVaughan]

We don't have an easy way to do r-devel revdep checks, but I did an r 4.3.1 revdepcheck across vctrs, dplyr, and tidyr and did not see any issues. The two failures were ggmap related false positives that I think are related to downloading things.

I have to imagine that if this was going to cause issues, then it would also show up on released versions of R with other kinds of non-list ALTREP objects, so I'm optimistic that this is working as expected

## revdepcheck results

We checked 4313 reverse dependencies (4310 from CRAN + 3 from Bioconductor), comparing R CMD check results across CRAN and dev versions of this package.

 * We saw 2 new problems
 * We failed to check 6 packages

Issues with CRAN packages are summarised below.

### New problems
(This reports the first line of each new failure)

* ggquiver
  checking examples ... ERROR

* SWMPrExtension
  checking examples ... ERROR

### Failed to check

* bayesdfa         (NA)
* loon.ggplot      (NA)
* loon.shiny       (NA)
* TriDimRegression (NA)
* triptych         (NA)
* vivid            (NA)