modularize optimization internals

[ben-schwen]

• Closes Using Map instead of lapply turns GForce off #5336
• Closes lapply GForce opt could work also without .SD #5032
• Closes Move GForce tests to own script #4305
• Towards GForce optimisation could be more smart #3815
• Closes GForce as.double / as.numeric #2934
• Closes benchmark regression #7404
• tests (a lot of them)

Adds arithmetic for GForce as demanded in #3815 but does not add support for blocks in j like d[, j={x<-x; .(min(x))}, by=y].

[codecov]

Codecov Report

❌ Patch coverage is 99.25094% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.01%. Comparing base (a325db9) to head (c5e95d2).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
R/data.table.R	99.59%	1 Missing ⚠️
R/test.data.table.R	95.23%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7401      +/-   ##
==========================================
- Coverage   99.02%   99.01%   -0.02%     
==========================================
  Files          87       87              
  Lines       16803    16895      +92     
==========================================
+ Hits        16640    16728      +88     
- Misses        163      167       +4     

☔ View full report in Codecov by Sentry.
📢 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=modular_gforce stopped early for DT[by,verbose=TRUE] improved in #6296
• HEAD=modular_gforce slower P<0.001 for memrecycle regression fixed in #5463
• HEAD=modular_gforce slower P<0.001 for setDT improved in #5427
  

Generated via commit c5e95d2

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

Task	Duration
R setup and installing dependencies	2 minutes and 57 seconds
Installing different package versions	44 seconds
Running and plotting the test cases	5 minutes and 3 seconds

[ben-schwen]

I'm also not sure about moving the tests to optimize.Rraw since this feels kind of wrong and not needed after introducing the new levels/optimization parameter to test.

[MichaelChirico]

Let's just drop this branch? since it's not yet supported.

[MichaelChirico]

might be worth benchmarking this (atime?)... written 14 years ago, I wonder if it's still true 5176108

[ben-schwen]

Its stil true with nrow=1e6, ncol=10, but no noticeable effect between master and this PR

Details

library(atime)
library(data.table)

pkg.path <- '.'
limit <- 10

# Package editing function for atime
pkg.edit.fun <- function(old.Package, new.Package, sha, new.pkg.path) {
  pkg_find_replace <- function(glob, FIND, REPLACE) {
    atime::glob_find_replace(file.path(new.pkg.path, glob), FIND, REPLACE)
  }
  Package_regex <- gsub(".", "_?", old.Package, fixed = TRUE)
  Package_ <- gsub(".", "_", old.Package, fixed = TRUE)
  new.Package_ <- paste0(Package_, "_", sha)
  pkg_find_replace(
    "DESCRIPTION",
    paste0("Package:\\s+", old.Package),
    paste("Package:", new.Package))
  pkg_find_replace(
    file.path("src", "Makevars.*in"),
    Package_regex,
    new.Package_)
  pkg_find_replace(
    file.path("R", "onLoad.R"),
    Package_regex,
    new.Package_)
  pkg_find_replace(
    file.path("R", "onLoad.R"),
    sprintf('packageVersion\\("%s"\\)', old.Package),
    sprintf('packageVersion\\("%s"\\)', new.Package))
  pkg_find_replace(
    file.path("src", "init.c"),
    paste0("R_init_", Package_regex),
    paste0("R_init_", gsub("[.]", "_", new.Package_)))
  pkg_find_replace(
    "NAMESPACE",
    sprintf('useDynLib\\("?%s"?', Package_regex),
    paste0('useDynLib(', new.Package_))
}

# Commits to compare
versions <- c(
  'master' = 'b3acef8',
  'PR' = '383b60a'
)

# Vary number of groups
N_groups <- 10^seq(1, 5, 0.25)

set.seed(42)

test_data <- lapply(setNames(nm = N_groups), function(n_groups) {
  n_rows <- 1e6
  n_cols <- 10
  dt <- data.table(grp = sample(n_groups, n_rows, replace = TRUE))
  for (i in seq_len(n_cols)) {
    set(dt, j = paste0("V", i), value = rnorm(n_rows))
  }

  dt
})

# Test 1: With optimize = 1 (GForce enabled)
gforce_opt1 <- atime_versions(
  pkg.path, N_groups,
  setup = {
    options(datatable.optimize = 1)
    DT <- test_data[[as.character(N)]]
  },
  expr = {
    data.table:::`[.data.table`(DT, , lapply(.SD, sum), by = grp)
  },
  seconds.limit = limit,
  verbose = TRUE,
  sha.vec = versions,
  pkg.edit.fun = pkg.edit.fun
)

# Test 2: With optimize = 0 (GForce disabled)
gforce_opt0 <- atime_versions(
  pkg.path, N_groups,
  setup = {
    options(datatable.optimize = 0)
    DT <- test_data[[as.character(N)]]
  },
  expr = {data.table:::`[.data.table`(DT, , lapply(.SD, sum), by = grp)
  },
  seconds.limit = limit,
  verbose = TRUE,
  sha.vec = versions,
  pkg.edit.fun = pkg.edit.fun
)

library(ggplot2)
opt1_data <- gforce_opt1$measurements
opt1_data$optimize <- "opt1"
opt0_data <- gforce_opt0$measurements
opt0_data$optimize <- "opt0"
combined_data <- rbind(opt1_data, opt0_data)

pdf("gforce_lapply_benchmark.pdf", width = 12, height = 6)

p <- ggplot(combined_data, aes(x = N, y = median, color = expr.name, linetype = optimize)) +
  geom_line(linewidth = 1) +
  geom_point(size = 2) +
  scale_x_log10("Number of groups") +
  scale_y_log10("Median time (seconds)") +
  labs(title = "DT[, lapply(.SD, sum), by=grp] Performance",
       subtitle = "Comparing optimize=0 vs optimize=1 across commits",
       color = "Version",
       linetype = "Optimization") +
  theme_bw() +
  theme(legend.position = "bottom")

print(p)

dev.off()

[MichaelChirico]

nice... wonder if benchmark.Rraw or something like that (a Wiki?) would be a good place to compile these type of low-level optimizations based on R's own logic...

(not required for this PR)

[MichaelChirico]

Does it make sense to have a script of things that are only testable with the R 4.0.0+ parser? I guess it's pretty rare...

Suggested change

	if (fun %iscall% "function") {
	if (fun %iscall% "function") { # NB: '\(x)' only exists pre-parser, so it's also covered

[MichaelChirico]

nit: i guess this is obsolete? there's no i in scope here and no eval() here

[MichaelChirico]

I changed another 2:length(...) to this seq.int() form, but I see 2:length(...) elsewhere and now I'm second-guessing -- I guess it's fine (better, even) to use the shorter 2:length(...)?

[MichaelChirico]

IINM isTRUE() is only needed if names() might be NA, which is possible in general but shouldn't be here. Let's remove it?

[MichaelChirico]

oh no, it's for the is.null(names(.)) case, got it.

[MichaelChirico]

?

Suggested change

	jl__hasname = nzchar(names(jl__))
	jl__hasname = nzchar(jl__names)

[MichaelChirico]

maybe?

Suggested change

	idx = unlist(lapply(jl__, function(x) is.name(x) && x == ".I"))
	idx = vapply_1b(jl__, identical, quote(.I))

[MichaelChirico]

what's the difference between is_valid and any_optimized?

[MichaelChirico]

this is_valid=FALSE; break style feels a bit like a goto statement where we have to jump past a bunch of other logic to see the flow; why not do early returns directly at the site of the "failure" instead?

Maybe something like this would help

unedited_expr = list(jsub=jsub, jvnames=oldjvnames, funi=funi, optimized=FALSE)

# ...

return(unedited_expr)

[MichaelChirico]

My memory is it's good to avoid creating lambdas if you can avoid it

Suggested change

	sum(vapply_1b(as.list(jsub), function(x) identical(x, quote(.SD)))) == 1L) {
	sum(vapply_1b(as.list(jsub), identical, quote(.SD))) == 1L) {

[MichaelChirico]

OK, I think last round of review here. Looking great!