Migrate most uses of `SETLENGTH` to the resizable API #7451

aitap · 2025-12-06T17:11:03Z

The new resizable vectors API wraps SETLENGTH, TRUELENGTH, and the GROWABLE_BIT. Since all the vectors we want to resize are now growable, this frees us from:

having to adjust the allocated memory counts in the finalizer
manual adjustment of TRUELENGTH for duplicated vectors (R resets TRUELENGTH to 0 when duplicating a growable vector)

On the other hand,

resizable vectors have to be specially allocated
resizing drops names, we have to reinstall them afterwards
data.table::truelength must be emulated using R_maxLength.

@jangorecki, I had to introduce copyAsGrowable so that adaptive frollapply would work. Would that be fine, or should we find a way to make that copy earlier?

@TysonStanley, if merged, this will be needed for cherry-picking into the patch branch.

There will be one more batch of fixes in src/dogroups.c that depends on both this PR (for resizable API) and #6694 (for the other use of TRUELENGTH).

Fixes: #990
Closes: #6697

Many thanks to Benjamin Schwendinger for proposing the API and Luke Tierney for making it a reality.

Thanks to Luke Tierney for introducing the API and helping with the migration.

Make sure to set the GROWABLE_BIT on the resizable vectors to avoid problems when they are duplicated or garbage-collected.

Now that data.table objects have the GROWABLE_BIT set, R will reset TRUELENGTH when duplicating them, causing our code to take a different branch.

Now that (1) we depend on R >= 3.4 and (2) data.table objects have the GROWABLE_BIT set, there is no need to adjust allocated memory counts by hand.

Since adaptive application of rolling functions requires us to resize the argument to match the window size, make sure to allocate it as such.

- Don't SET_TRUELENGTH by hand. All of our resizable vectors now have the GROWABLE_BIT set, so when they are duplicated, TRUELENGTH is reset to 0. - Use a combination of R_isResizable and R_maxLength to replace other uses of TRUELENGTH.

github-actions · 2025-12-06T17:29:28Z

No obvious timing issues in HEAD=resizable-API

Generated via commit 5caf9cc

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

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

codecov · 2025-12-06T18:40:19Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.06%. Comparing base (a014e38) to head (5caf9cc).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7451      +/-   ##
==========================================
- Coverage   99.07%   99.06%   -0.01%     
==========================================
  Files          85       86       +1     
  Lines       16610    16600      -10     
==========================================
- Hits        16456    16445      -11     
- Misses        154      155       +1

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

ben-schwen

LGTM, TY!

jangorecki · 2025-12-07T18:50:37Z

@aitap place for allocate as resizeable is perfect.

I am not sure about by.column usage there, for by.column=TRUE it seems that columns will not be resizeable but only list of pointers to columns. Am I wrong?

HughParsonage · 2025-12-07T15:53:00Z

src/frollapply.c

-  }
-  return x;
+SEXP copyAsGrowable(SEXP x, SEXP by_column) {
+  if (LOGICAL_RO(by_column)[0])


Guard length(by_column) == 0 or use a different macro. (I can see the R logic requires TRUE or FALSE but it's best not to trust anything about a SEXP coming from R.)

HughParsonage · 2025-12-07T16:35:36Z

src/frollapply.c

  for (int i = 0; i < ncol; i++) {
    SEXP d = VECTOR_ELT(dest, i);
-    SETLENGTH(d, nrow);
+    R_resizeVector(d, nrow);


Probably overly defensive but assume R_resizeVector strips all but values. So names might be lost in corner cases.

Lost naming is fine in this use case

HughParsonage · 2025-12-07T22:58:30Z

src/subset.c


  int overAlloc = checkOverAlloc(GetOption1(install("datatable.alloccol")));
-  SEXP ans = PROTECT(allocVector(VECSXP, LENGTH(cols)+overAlloc)); nprotect++;  // doing alloc.col directly here; eventually alloc.col can be deprecated.
+  SEXP ans = PROTECT(R_allocResizableVector(VECSXP, LENGTH(cols)+overAlloc)); nprotect++;  // doing alloc.col directly here; eventually alloc.col can be deprecated.


Is the comment still applicable?

It's been there since 8e272c8. I guess not. Should I remove it altogether?

HughParsonage · 2025-12-07T23:01:43Z

src/data.table.h

+# define R_duplicateAsResizable(x) R_duplicateAsResizable_(x)
+# define R_resizeVector(x, newlen) SETLENGTH(x, newlen)
+# define R_maxLength(x) TRUELENGTH(x)
+# define R_isResizable(x) R_isResizable_(x)


Does this need

if (ALTREP(x)) return false;

Good catch. We also checked for ALTREP(x) in the proposed version

We could also teach copyAsPlain to allocate resizable vectors and use it instead of R_duplicateAsResizable, but for now this works.

HughParsonage · 2025-12-07T23:04:04Z

src/data.table.h

+  }
+# define R_duplicateAsResizable(x) R_duplicateAsResizable_(x)
+# define R_resizeVector(x, newlen) SETLENGTH(x, newlen)
+# define R_maxLength(x) TRUELENGTH(x)


return R_isResizable_(x) ? (R_xlen_t)TRUELENGTH(x) : 0;

might be nicer

WRE doesn't document what's the R_maxLength() of a non-resizable vector, but in R-devel this function returns xlength(), not 0 (which necessitates R_isResizable and the LEVELS trick below). So far we're just lucky/careful that the TRUELENGTH() calls replaced with R_maxLength() don't contradict each other too much.

I can make a more precise backport of R_maxLength(), of course.

HughParsonage · 2025-12-07T23:05:39Z

src/data.table.h

+    return ret;
+  }
+# define R_duplicateAsResizable(x) R_duplicateAsResizable_(x)
+# define R_resizeVector(x, newlen) SETLENGTH(x, newlen)


Should this be guarded by R_isResizable_(x) and new_len <= TRUELENGTH(x), and no attributes?

Yes. It would be cleaner to guard for them, so we not only catch them via R-devels runner

if (!R_isResizable(x)) error(_("not a resizable vector")); if (newlen > XTRUELENGTH(x)) error(_("'newlen' is too large"));

I don't think we should introduce extra checks here, this is called in tight loops, and it is better to check before those loops.

What's a better benchmark for this, frollapply(adaptive = TRUE), or foo[bar, by = baz]?

frolladaptive willa stress more, unless grouping is by unique column, then probably similarly

That's what the function does in R-devel.

aitap · 2025-12-08T14:41:27Z

I am not sure about by.column usage there, for by.column=TRUE it seems that columns will not be resizeable but only list of pointers to columns. Am I wrong?

by_column here follows the by.column argument of frollapply. When by_column is TRUE, the x argument is a single column, not a list of column pointers. Otherwise x is a list and its elements are columns that need to be made resizable.

If this sounds confusing, let's rename the argument.

aitap added 7 commits December 6, 2025 19:55

Use the experimental resizable vectors API

23c7b3b

Thanks to Luke Tierney for introducing the API and helping with the migration.

Backport the resizable API

c3e8f0c

Make sure to set the GROWABLE_BIT on the resizable vectors to avoid problems when they are duplicated or garbage-collected.

test 2291.1: misleading TRUELENGTH now impossible

04437b6

Now that data.table objects have the GROWABLE_BIT set, R will reset TRUELENGTH when duplicating them, causing our code to take a different branch.

Drop the finalizer

2fa7728

Now that (1) we depend on R >= 3.4 and (2) data.table objects have the GROWABLE_BIT set, there is no need to adjust allocated memory counts by hand.

frollapply(adaptive=TRUE): resizable temporaries

332bfba

Since adaptive application of rolling functions requires us to resize the argument to match the window size, make sure to allocate it as such.

Drop remaining uses of TRUELENGTH from assign.c

be921b1

- Don't SET_TRUELENGTH by hand. All of our resizable vectors now have the GROWABLE_BIT set, so when they are duplicated, TRUELENGTH is reset to 0. - Use a combination of R_isResizable and R_maxLength to replace other uses of TRUELENGTH.

Drop test for TRUELENGTH from init.c

c1c4f76

aitap requested review from HughParsonage and MichaelChirico as code owners December 6, 2025 17:11

aitap marked this pull request as draft December 6, 2025 17:29

aitap added 2 commits December 6, 2025 21:29

Better backport of R_isResizable()

87315a4

Placate rchk

2dd68e8

aitap marked this pull request as ready for review December 6, 2025 18:42

ben-schwen approved these changes Dec 7, 2025

View reviewed changes

HughParsonage reviewed Dec 7, 2025

View reviewed changes

ben-schwen mentioned this pull request Dec 8, 2025

Growable vectors with R supporting growable vectors #7343

Closed

aitap added 4 commits December 8, 2025 16:46

copyAsGrowable: don't crash on 0-len argument

7395eb1

Move the resizable allocation functions to utils.c

40b92d1

duplicateAsResizable: refuse ALTREP objects

af91fe9

maxLength: return xlength if non-resizable

5caf9cc

That's what the function does in R-devel.

Migrate most uses of SETLENGTH to the resizable API #7451

Are you sure you want to change the base?

Migrate most uses of SETLENGTH to the resizable API #7451

Uh oh!

Conversation

aitap commented Dec 6, 2025

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

ben-schwen left a comment

Choose a reason for hiding this comment

Uh oh!

jangorecki commented Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

aitap Dec 8, 2025 • 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!

HughParsonage Dec 7, 2025 • 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!

aitap commented Dec 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Migrate most uses of `SETLENGTH` to the resizable API #7451

Migrate most uses of `SETLENGTH` to the resizable API #7451

github-actions bot commented Dec 6, 2025 •

edited

Loading

codecov bot commented Dec 6, 2025 •

edited

Loading

jangorecki commented Dec 7, 2025 •

edited

Loading

aitap Dec 8, 2025 •

edited

Loading

HughParsonage Dec 7, 2025 •

edited

Loading