modularize optimization internals #7401

ben-schwen · 2025-10-28T17:32:57Z

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 · 2025-10-28T17:51:29Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.94%. Comparing base (851467f) to head (8129198).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7401      +/-   ##
==========================================
- Coverage   98.96%   98.94%   -0.02%     
==========================================
  Files          87       87              
  Lines       16734    16819      +85     
==========================================
+ Hits        16560    16642      +82     
- Misses        174      177       +3

☔ 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 · 2025-10-28T17:52:01Z

HEAD=modular_gforce slower P<0.001 for memrecycle regression fixed in #5463

Generated via commit 8129198

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

Task	Duration
R setup and installing dependencies	2 minutes and 53 seconds
Installing different package versions	21 seconds
Running and plotting the test cases	5 minutes and 4 seconds

man/test.Rd

R/data.table.R

ben-schwen · 2025-11-02T19:05:06Z

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.

NEWS.md

MichaelChirico · 2026-01-07T08:03:15Z

inst/tests/tests.Rraw

+test(2358.33,optimize=0:2, dt[, sum(a) / .N, b, verbose=TRUE], output=out)
+test(2358.34,optimize=0:2, dt[, mean(a) * 2L + sum(a), b, verbose=TRUE], output=out)
+test(2358.35,optimize=0:2, dt[, list(range=max(a)-min(a), avg=mean(a)), by=b, verbose=TRUE], output=out)
+test(2358.36,optimize=0:2, dt[, .(max(a)-sqrt(min(a))), by=b, verbose=TRUE], output="GForce FALSE")


reading this inspires some stretch goals for future work:

GForce for simple regression coefficients ((mean(x*y) - mean(x)*mean(y))/(mean(x^2) - mean(x)^2))

sqrt(min(a)) could do GForce aggregation

list(mean(x)/sd(x), mean(x), sd(x)) could only compute mean(), sd() once

max(x) - min(x) could compute range() with GForce and then derive min, max (i.e., single pass instead of two)

Co-authored-by: Michael Chirico <[email protected]>

ben-schwen · 2026-01-07T10:51:29Z

@MichaelChirico I'm also not 100% convinced about the new optimize.Rraw. I guess the whole idea was that we could simply run the script multiple times with different optimization levels. This need was eliminated by adding the optimize parameter to test() which somehow feels cleaner.

Co-authored-by: Michael Chirico <[email protected]>

MichaelChirico · 2026-01-07T17:35:53Z

@MichaelChirico I'm also not 100% convinced about the new optimize.Rraw. I guess the whole idea was that we could simply run the script multiple times with different optimization levels. This need was eliminated by adding the optimize parameter to test() which somehow feels cleaner.

I see. I still like the idea of a separate script -- the more we peel out of the behemoth tests.Rraw, the better. "eventually" it would be nice to have most tests live in purpose-made test scripts, IMO.

MichaelChirico · 2026-01-07T17:39:45Z

inst/tests/tests.Rraw

  test(2357.2, fread(paste0("file://", f)), DT)
 })
+
+# gforce should also work with Map in j #5336


one last idea -- what happens when the grouping column is part of the aggregation in j?

DT[, .(sum(b) - mean(a)), by=b]

MichaelChirico · 2026-01-07T17:42:40Z

R/data.table.R

-      }
-    }
+  # Call extracted GForce optimization function
+  if ( getOption("datatable.optimize")>=1L && (is.call(jsub) || (is.name(jsub) && jsub %chin% c(".SD", ".N"))) ) {


let's move the latter test into .attempt_optimize? Just a quick escape at the top. I think it will look cleaner, and the checks for .SD/.N feel like they belong there, anyway.'

Edit: I see that it's feeding in to the verbose message below... I think that can also be moved into the helpers.

MichaelChirico · 2026-01-07T17:44:05Z

R/data.table.R

-        }
-      }
-    }
+  # Call extracted GForce optimization function


this comment feels ephemeral -- it explains what's happening in this PR, but will look out of place in 5 years.

Maybe something like

Determine GForce-optimized query

MichaelChirico · 2026-01-07T17:49:38Z

R/data.table.R

+}
+
+# attempts to optimize j expressions using lapply, GForce, and mean optimizations
+.attempt_optimize = function(jsub, jvnames, sdvars, SDenv, verbose, i, byjoin, f__, ansvars, use.I, lhs, names_x, envir) {


really like how clean this is 👍

MichaelChirico · 2026-01-07T17:53:47Z

R/data.table.R

+  GForce = FALSE
+
+  # FR #971, GForce kicks in on all subsets, no joins yet. Although joins could work with
+  # nomatch=NULL even now.. but not switching it on yet, will deal it separately.


convert to a TODO?

MichaelChirico · 2026-01-07T17:55:25Z

R/data.table.R

+
+  # FR #971, GForce kicks in on all subsets, no joins yet. Although joins could work with
+  # nomatch=NULL even now.. but not switching it on yet, will deal it separately.
+  if (getOption("datatable.optimize")>=2L && !is.data.table(i) && !byjoin && length(f__)) {


recommend unnesting:

if (getOption("datatable.optimize") < 2L || is.data.table(i) || byjoin || !length(f__)) return(list(GForce=FALSE, jsub=jsub))

MichaelChirico · 2026-01-07T17:56:02Z

R/data.table.R

+
+# Old mean() optimization fallback when GForce is not used
+.optimize_mean = function(jsub, SDenv, verbose, GForce) {
+  if (!GForce && !is.name(jsub)) {


same recommendation around unnesting:

if (GForce || is.name(jsub)) return(jsub)

MichaelChirico

About halfway done reading the implementation now. Thanks for your patience with the review! I'm really excited for this to get finished :)

MichaelChirico · 2026-01-07T18:01:01Z

R/data.table.R

+
+# Optimize expressions using GForce (C-level optimizations)
+# This function replaces functions like mean() with gmean() for fast C implementations
+.optimize_gforce = function(jsub, SDenv, verbose, i, byjoin, f__, ansvars, use.I, lhs, names_x, envir) {


one thing that comes to mind seeing such a long signature -- using a "struct" instead of passing individual arguments, e.g.

https://stackoverflow.com/questions/31864162/what-are-the-pros-and-cons-of-using-a-struct-argument-v-s-multiple-parameters

There may be some possibility to make the code easier to understand if some arguments are grouped or combined.

Not a requirement but something to ponder.

modular optimization paths - init

ebd152d

ben-schwen added 13 commits October 29, 2025 09:17

make linter happy

71b21ab

move tests

8a9e727

add lapply(list(col1, col2, ...), fun) pattern

04e5782

turn on optimization

a8dde19

add type conversion support to GForce

67f2874

remove stale branch

2876ebe

add tests

c445c38

update man

5410e31

merge tests

dece1c6

polish test fun

5e1789d

add arithmetic

62f1c48

add AST walker and update tests

c47ec27

add tests

1d324d6

ben-schwen marked this pull request as ready for review November 2, 2025 18:01

ben-schwen requested a review from MichaelChirico as a code owner November 2, 2025 18:01

ben-schwen added 2 commits November 2, 2025 19:30

Merge branch 'master' into modular_gforce

6b54c1e

add NEWS

22cf35e

jangorecki reviewed Nov 2, 2025

View reviewed changes

man/test.Rd Outdated Show resolved Hide resolved

jangorecki reviewed Nov 2, 2025

View reviewed changes

R/data.table.R Outdated Show resolved Hide resolved

ben-schwen mentioned this pull request Nov 2, 2025

benchmark regression #7404

Closed

jangorecki reviewed Nov 3, 2025

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

ben-schwen added 5 commits November 3, 2025 09:45

make function name in massageSD more expressive

25a7e2e

rename levels argument to optimization

eb8056c

update docs

4544398

restore test nums

d40edb8

remove double tests

5e7efb7

MichaelChirico reviewed Jan 7, 2026

View reviewed changes

ben-schwen and others added 12 commits January 7, 2026 10:20

add space

fe525bf

Co-authored-by: Michael Chirico <[email protected]>

reference deletion of tests

71b9838

Co-authored-by: Michael Chirico <[email protected]>

reference deletion of tests2

494cfe2

Co-authored-by: Michael Chirico <[email protected]>

add comment about removed tests

6f42ff5

Co-authored-by: Michael Chirico <[email protected]>

add comment about optimization level comparison

ac306eb

Co-authored-by: Michael Chirico <[email protected]>

add comment about removed test

431dfc2

Co-authored-by: Michael Chirico <[email protected]>

fix typo

158136b

remove doubled test

0c2f61f

add comment

2c7ebaf

update subsuming comments

371e246

add subsuming comments

e2694e1

finish double checking of moving tests

da771d4

ben-schwen and others added 6 commits January 7, 2026 11:53

make optimize more robust

af15282

Co-authored-by: Michael Chirico <[email protected]>

add comment about removing tests in benchmark.Rraw

b61f280

be clearer in NEWS

d8e34d3

add nocovs for errors

c5fb65a

add unwrapper for conversions

9f0e5cf

add more tests

8129198

MichaelChirico reviewed Jan 7, 2026

View reviewed changes

modularize optimization internals #7401

Are you sure you want to change the base?

modularize optimization internals #7401

Conversation

ben-schwen commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ben-schwen commented Nov 2, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben-schwen commented Jan 7, 2026

Uh oh!

MichaelChirico commented Jan 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelChirico Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelChirico left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ben-schwen commented Oct 28, 2025 •

edited

Loading

codecov bot commented Oct 28, 2025 •

edited

Loading

github-actions bot commented Oct 28, 2025 •

edited

Loading

MichaelChirico Jan 7, 2026 •

edited

Loading