GForce optimisation could be more smart

[jangorecki]

d = data.table(x=1:4, y=1:2)
options(datatable.verbose=TRUE)
d[, j=.(min(x)), by=y]                       ## GForce TRUE
d[, j=.(min(x), mean(x)/min(x)), by=y]       ## GForce FALSE
d[, j={.(min(x))}, by=y]                     ## GForce TRUE
d[, j={x<-x; .(min(x))}, by=y]               ## GForce FALSE
d[, j={mn<-min(x); .(mn, mean(x)/mn)}, by=y] ## GForce FALSE
options(datatable.verbose=FALSE)

all those cases could be optimised

[ben-schwen]

I think another case of this also appears in the db-benchmark (now resurrected by duckdblabs at https://github.com/duckdblabs/db-benchmark) as Q2 advanced group by

Since DT[, .(range_v1_v2=max(v1, na.rm=TRUE)-min(v2, na.rm=TRUE)), by=id3] could be rewritten using chaining.

@jangorecki is query rewriting something viable in the benchmarks or does this lead it in the wrong direction?

With current master this gives a good speedup

library(data.table)
n = 1e7
DT=data.table(v1=rnorm(n), v2=rnorm(n), id3=sample(1e4, n, TRUE))
bench::mark(
  now = DT[, .(range_v1_v2=max(v1, na.rm=TRUE)-min(v2, na.rm=TRUE)), by=id3],
  gforced = DT[, .(max(v1, na.rm=TRUE), min(v2, na.rm=TRUE)), id3][, .(range_v1_v2=V1-V2), by=id3]
)
#> Warning: Some expressions had a GC in every iteration; so filtering is disabled.
#> # A tibble: 2 × 6
#>   expression      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 now           316ms    328ms      3.05    40.3MB     0   
#> 2 gforced       126ms    135ms      6.57     268MB     8.22

[jangorecki]

I would keep it as it is, benchmark also compares query syntax

[ColeMiller1]

@ben-schwen somewhat related to your note, #4412 has some aspects of this but is more focused on logical expression evaluations. From the PR (note the 3 gForce optimized functions combined via |):

bench::mark(
  new_hav = dt[,
               j = .SD,
               by = x,
               having = .N < 2L | sum(y) > 11 | median(y) < 0.7
               ],
  use_I = dt[dt[, .I[.N < 2L | sum(y) > 11 | median(y) < 0.7], by = x]$V1],
  time_unit = 's'
)

##   expression    min median `itr/sec` mem_alloc
##   <bch:expr>  <dbl>  <dbl>     <dbl> <bch:byt>
## 1 new_hav    0.0102 0.0112    81.4      6.97MB
## 2 use_I      1.21   1.21       0.829    3.21MB

I believe I had done research and got your query to work using GForce using similar methods as the PR. I'd have to review my other branches more, but it's doable to optimize without changing the way the query looks. As your benchmark notes, it's much more memory intensive this route but is generally faster.

As an aside, should this issue be closed? I am not sure how to optimize {min_x = min(x); .(min_x, min_x/mean(x))} and there are other very similar issues about optimizing expressions to GForce optimizations.

[clerousset]

Can I add to this topic that :

d = data.table(x=1:4, y=1:2)
options(datatable.verbose=TRUE)
  d[, head(x, 1), by=y]        ## GForce TRUE
  d[, utils::head(x, 1), by=y] ## GForce FALSE

?
As a simple user, sometimes we've been told in some R guidelines (maybe wrongly) to always put the package before, even for basic packages like utils

(behaviour checked for data.table 6f43a96)

[MichaelChirico]

Can I add to this topic that :

d = data.table(x=1:4, y=1:2)
options(datatable.verbose=TRUE)
  d[, head(x, 1), by=y]        ## GForce TRUE
  d[, utils::head(x, 1), by=y] ## GForce FALSE

? As a simple user, sometimes we've been told in some R guidelines (maybe wrongly) to always put the package before, even for basic packages like utils

(behaviour checked for data.table 6f43a96)

that will never be supported. authors might need to invoke the "real" head to get S3 dispatch for example. GForce head completely skips method dispatch of course

my snarky opinion is that the authors telling you to 'always qualify namespace' should stick to writing C++ if that's what they want so badly 🙃