Use merge commits for atime tests + cleanup

[tdhock]

Closes #7363

Change an atime historical commit from a commit inside a PR branch, to the merge commit of that PR, and deleted branches, in

• Move some setDT validation checks to C #5427
• towards #4200, overhead in dogroups #4558
• Skip sorting already sorted #4501
• Removes unnecessary data.table call from as.data.table.array #7019
• Use faster base implementation for isoweek #7144
• Teach forder to re-use existing key and index attributes instead of sorting from scratch #4386

also some more general atime cleanup which should not affect test results:

• reduce density of elements in N (reduce false positives)
• move expr to last arg of atime_test, so easy to comment historical versions.
• added and fixed comments to document commit ID sources.

[github-actions]

• HEAD=atime-merge much slower for isoweek improved in #7144
• HEAD=atime-merge slower P<0.001 for setDT improved in #5427
  

Generated via commit c9ebb95

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

Task	Duration
R setup and installing dependencies	3 minutes and 2 seconds
Installing different package versions	4 minutes and 8 seconds
Running and plotting the test cases	6 minutes and 52 seconds

[tdhock]

I checked atime results before and after this PR, and they look consistent. The biggest difference is in this test, which is normal because old test code was starting at a custom N=1e3 whereas new test code uses the standard N=1e1 for this case (custom N code deleted).

Above is a screenshot from

which I made using data from

• old (recent PR with atime results) https://github.com/Rdatatable/data.table/actions/runs/23863221564/artifacts/6226911320 add PR7401 merge sha to DT[by] fixed in #4558 test #7692 (comment) atime-results-old.zip
• new (this PR) https://github.com/Rdatatable/data.table/actions/runs/24704372856/artifacts/6547259271 Use merge commits for atime tests + cleanup #7701 (comment) atime-results-new.zip

library(data.table)
bench_dt <- data.table(test_code=c("old","new"))[, {
  result_dir <- paste0("atime-results-",test_code)
  tests.RData <- file.path(result_dir, "tests.RData")
  load(tests.RData)
  bench.dt[, .(Test, unit, N, empirical, expr.name)]
}, by=test_code]

library(ggplot2)
gg <- ggplot()+
  geom_line(aes(
    N, empirical, color=expr.name),
    data=bench_dt)+
  scale_color_manual(values=atime:::default.version.colors)+
  facet_grid(unit ~ Test + test_code, scales="free", labeller=label_both)+
  scale_x_log10()+
  scale_y_log10()
png("atime-results.png", width=120, height=8, units="in", res=100)
print(gg)
dev.off()

[tdhock]

I updated https://github.com/Rdatatable/data.table/wiki/Performance-testing to mention

• only use merge commits on master (not other commits in PR branches which will be deleted)
• put expr as last argument of atime_test() so we can easily comment other args.

[tdhock]

@Anirban166 @MichaelChirico @aitap can you please review?

[Anirban166]

The changes to tests.R and the wiki look good to me. Thanks!

[aitap]

A commit is "on a branch" if its common ancestor with the HEAD of the branch is the commit itself:

library(codetools)
w = makeCodeWalker(leaf = \(e, w)
  if (is.character(e) && nchar(e) > 4 && grepl('^[0-9a-f]+$', e, perl = TRUE)) {
    mergebase = system2('git', c('merge-base', 'master', e), stdout = TRUE)
    if (mergebase != e) message(sprintf(
      "merge-base(master, %s) = %s", e, mergebase
    ))
  }
)
for (e in parse('.ci/atime/tests.R')) walkCode(e, w)

Looks like we need just one more fix.

[aitap]

I think that the merge commit of the regression-introducing PR is the better choice here.

Suggested change

	Regression = "e5e10a09b32f851465790cef98526ab63d5cee3a", # Parent of first commit (https://github.com/Rdatatable/data.table/commit/4acabf0bf8541f6db629bccc6d5f7c806199416a) of fix PR (https://github.com/Rdatatable/data.table/pull/7480/commits), another choice would be 6f49bf1935a3009e85ea1e6f9752ff68ffa47d9b which is merge commit in regression PR https://github.com/Rdatatable/data.table/pull/6890
	Regression = "6f49bf1935a3009e85ea1e6f9752ff68ffa47d9b", # Merge commit of the regression PR https://github.com/Rdatatable/data.table/pull/6890

[Anirban166]

Looks much cleaner!

[aitap]

3630413 was also on the PR branch, not on master

Suggested change

	PR = "3630413ae493a5a61b06c50e80d166924d2ef89a", # Close-to-last merge commit in the PR.
	PR = "e0abdfcd79ba30efcf679e33cbb8eba897a46f9c", # merge commit of PR6393