copy(): only overwrite the list when necessary, consider the attributes

[aitap]

Rewrite copy() in C so that instead of calling y[] <- lapply(y, reallocate) we can consider each individual list element and only call SET_VECTOR_ELT when necessary. This avoids the problem with xgboost's special altlist that must never be altered.

Also walk the attributes of the object being copied, look for data.tables and reallocate them. Maybe that's not necessary (copy() has been working fine without it) and can be taken out.

Test 2224.3 points out an important detail: when y is a LISTSXP pairlist, y[] <- lapply(y, reallocate) makes it into a VECSXP list instead. It turns out we've been relying on copy() automatically coercing pairlist columns into vector lists when constructing data.tables. Should this become more explicit and be moved somewhere into the data.table() constructor, leaving copy() to just duplicate + reallocate data.tables?

Unfortunately this is hard to test without loading xgboost or creating a different altclass.

Fixes: #7456

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.02%. Comparing base (895067d) to head (3439810).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7820   +/-   ##
=======================================
  Coverage   99.02%   99.02%           
=======================================
  Files          88       88           
  Lines       17266    17282   +16     
=======================================
+ Hits        17097    17113   +16     
  Misses        169      169           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

• HEAD=fix7456 much slower for transform improved in #5493
• HEAD=fix7456 much slower for as.data.table.array improved in #7019
  

Generated via commit 3439810

Download link for the artifact containing the test results: ↓ atime-results.zip

Task	Duration
R setup and installing dependencies	6 minutes and 28 seconds
Installing different package versions	12 minutes and 32 seconds
Running and plotting the test cases	6 minutes and 55 seconds

[ben-schwen]

What immediately comes to my mind. How fast is this in terms of speed?

[ben-schwen]

should we add a test for nested tables?

inner = data.table(a=1)
DT = data.table(x=1:2, y=list(inner, inner))
DT2 = copy(DT)
test(2379.1, truelength(DT2$y[[1L]]) > 0L)  # is FALSE in current master
test(2379.2, DT2$y[[1L]][, b := 2][, b], 2) # warns in current master

[ben-schwen]

what if we have multiple attributes like:

x = list()
attr(x, 'foo') = 1:2
attr(x, 'dt') = data.table()
truelength(attr(copy(x), 'dt'))

[aitap]

atime::atime_versions(
 '.', N = 10^seq(0, 3, length.out = 10),
 setup = x <- (\(n) if (n > 0) list(Recall(n-1)) else replicate(100, vector('list', 100), FALSE) |>
as.data.table())(N),
 expr = data.table::copy(x),
 sha.vec = c(master = 'master', Ccopy = 'fix7456', 'no att' = 'fix7456noatt'),
 seconds.limit = 3, times = 20,
 pkg.edit.fun = '.ci/atime/tests.R' |> parse() |> _[[6]][[3]]$pkg.edit.fun |> eval()
) -> ret

Strange. With a contrived example I can show the new `copy` being faster, but there's a constant-factor slowdown unrelated to attribute scanning. Will have to investigate more.

[ben-schwen]

Not sure how atime works internally, but could this be because of some C interface overhead?

[ben-schwen]

Change LGTM. Might want to look deeper into timings before merging though

[aitap]

The timing difference was due to lists inside the data.table, not walked by the previous copy() function.

List of data.tables, the new code is slightly faster:

atime::atime_versions(
 '.', N = 10^seq(0, 3, length.out = 10),
 setup = { x <- replicate(N, as.data.table(vector('list', 1000), FALSE)); gc(full = TRUE) },
 expr = data.table::copy(x),
 sha.vec = c(master = 'master', Ccopy = 'fix7456', 'no att' = 'fix7456noatt'),
 seconds.limit = 3, times = 20,
 pkg.edit.fun = '.ci/atime/tests.R' |> parse() |> _[[6]][[3]]$pkg.edit.fun |> eval()
) -> ret1

data.table of lists, the new code is slower (because the old code didn't walk the list columns):

atime::atime_versions(
 '.', N = 10^seq(0, 3, length.out = 10),
 setup = { x <- as.data.table(replicate(N, vector('list', 100), FALSE)); gc(full = TRUE) },
 expr = data.table::copy(x),
 sha.vec = c(master = 'master', Ccopy = 'fix7456', 'no att' = 'fix7456noatt'),
 seconds.limit = 3, times = 20,
 pkg.edit.fun = '.ci/atime/tests.R' |> parse() |> _[[6]][[3]]$pkg.edit.fun |> eval()
) -> ret2

[ben-schwen]

TY. Great work, feel free to merge