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

* Never clone when `owned`

* NEWS bullet

* Add number

Author: DavisVaughan

NEWS.md

@@ -1,5 +1,8 @@
 # vctrs (development version)
 
+* Fixed a performance issue with `vec_c()` and ALTREP vectors (in particular,
+  the new ALTREP list vectors in R-devel) (#1884).
+
 * Fixed an issue with complex vector tests related to changes in R-devel
   (#1883).
 

src/owned.h

@@ -11,15 +11,8 @@ static inline enum vctrs_owned vec_owned(SEXP x) {
 }
 
 // Wrapper around `r_clone_referenced()` that only attempts to clone if
-// we indicate that we don't own `x`, or if `x` is ALTREP.
-// 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.
+// we indicate that we don't own `x`
 static inline SEXP vec_clone_referenced(SEXP x, const enum vctrs_owned owned) {
-  if (ALTREP(x)) {
-    return r_clone_referenced(x);
-  }
-
   if (owned == VCTRS_OWNED_false) {
     return r_clone_referenced(x);
   } else {

src/slice-assign.c

@@ -98,9 +98,11 @@ r_obj* vec_assign_switch(r_obj* proxy,
 // on a number of factors.
 //
 // - If a fallback is required, the `proxy` is duplicated at the R level.
-// - 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 r_obj* itself.
+// - If `owned` is `VCTRS_OWNED_true`, the `proxy` is not duplicated. If the
+//   `proxy` happens to be an ALTREP object, materialization will be forced when
+//   we do the actual assignment, but this should really only happen with
+//   cheap-to-materialize ALTREP "wrapper" objects since we've claimed that we
+//   "own" the `proxy`.
 // - If `owned` is `VCTRS_OWNED_false`, the `proxy` is only
 //   duplicated if it is referenced, i.e. `MAYBE_REFERENCED()` returns `true`.
 //