GForce optimisation could be more smart

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

@jangorecki

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

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

@ben-schwen

@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.

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)

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 🙃

@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)

@clerousset R 2.14.0 introduced namespaces as a feature, so conflicting names can be resolved from various namespaces, but it is a another feature of R that we can attach namespaces (library() / require() / NAMESPACE file) and do not have to prefix each function call with its namespace.
I can imagine there are some use cases to do it this, like when your dependencies chain (and/or their dependencies chain, and/or their dependencies chain, and/or ...) is so wildly out of control that you cannot be sure if in future functions like head won't be added to any of its dependencies (and/or any of its dependencies, and/or any of its dependencies, and/or ...).
But if that is your problem then it comes bundled with many others problems (and for robust and production grade costs definitely make sense to rethink your strategy), and then much better way to solve this particular one is to explicitly state imported functions in NAMESPACE file.

Navigation Menu

Repository navigation

GForce optimisation could be more smart #3815

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

GForce optimisation could be more smart #3815

Description

Activity

ben-schwen commented on Apr 15, 2023

jangorecki commented on Apr 15, 2023

ColeMiller1 commented on Apr 16, 2023

clerousset commented on Jan 25, 2024

MichaelChirico commented on Jan 25, 2024

jangorecki commented on Jun 26, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions