Skip to content

Growable vectors with R supporting growable vectors #7343

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 25 commits into
base: truehash
Choose a base branch
from
Open

Growable vectors with R supporting growable vectors #7343

wants to merge 25 commits into from
+95 −106

Conversation

ben-schwen
Copy link
Member

Towards #6180
Depends on #6694
Supersedes #6697

aitap and others added 14 commits December 29, 2024 19:10
@aitap
Introduce the growable vector API
421170e
@aitap
Use growable_* instead of SETLENGTH et al in fread
3b31850
@aitap
Switch shallow() to use growable_allocate()
f927451 
The resulting data.tables now have GROWABLE_BIT set, therefore:
 - the finalizer is not needed on R >= 3.4
 - duplicates of data.tables (which are not over-allocated) now have
   TRUELENGTH of 0 instead of whatever it was before, which is detected
   earlier in selfrefok()

As a result, assign.c only uses TRUELENGTH on R < 3.4.
@aitap
Use growable_allocate() in subsetDT()
e243ece
@aitap
Use growable_* instead of SETLENGTH in dogroups()
06e7db7 
Since dogroups() relies on being able to shrink vectors it hasn't
allocated, introduce make_growable() and is_growable() to adapt. Since
dogroups() relies on SD and SDall having shared columns, use the
setgrowable() wrapper on the R side at the time when SD and SDall are
being created.

(In the ALTREP case, setgrowable() will re-create the columns.)
@aitap
Mark internal_error*() as NORET
7b9b141 
This helps avoid false positive warnings about unreachable places in the code.
@aitap
ALTREP implementation of growable_*
61fdc06 
Currently broken:
- 1 errors out of 11637
 - won't work with expression vectors at all
@aitap @ben-schwen
Drop redundant variable definitions
c16fd97 
Co-Authored-By: Benjamin Schwendinger <[email protected]>
@ben-schwen
Merge branch 'truehash' into growable_refactor
deaa0f7
@ben-schwen
update growable for patch
cebe420
@ben-schwen
tweak patch update
2b5444b
@ben-schwen
reset truelength definition
248bab7
@ben-schwen
remove last TRUELENGTH
50998be
@ben-schwen
update setgrowable
c2c3bfd
@ben-schwen ben-schwen changed the base branch from master to truehash September 25, 2025 20:47
@jangorecki
Copy link
Member

jangorecki commented Sep 25, 2025 •
edited
Loading

SETLENGTH is left in frollapply.c rather than growable_resize(), is it intentional?
It runs for every observation so would be nice to have as little overhead like it is here now. Especially when we know what's the max size and that we will not attempt to resize to larger than that...

aitap
aitap reviewed Sep 25, 2025
View reviewed changes
@ben-schwen @aitap
Update src/data.table.h
78f45f8 
remove growable_altrep define

Co-authored-by: aitap <[email protected]>
ben-schwen
ben-schwen commented Sep 26, 2025
View reviewed changes
ben-schwen
ben-schwen commented Sep 26, 2025
View reviewed changes
@ben-schwen
Copy link
Member Author

SETLENGTH left in frollapply.c rather than growable_resize(), is it intentional? It runs for every observation so would be nice to have as little overhead as possible. Especially when we know what's the max size and that we will not attempt to resize to larger than that...

Didn't touch that yet. Not sure if exchanging SETLENGTH with growable_resize will do the full trick since the function dispatch in frollapply is quite cumbersome and its hard to grasp whether we end up with a DT or a vector.

@jangorecki
Copy link
Member

jangorecki commented Sep 26, 2025 •
edited
Loading

SETLENGTH left in frollapply.c rather than growable_resize(), is it intentional? It runs for every observation so would be nice to have as little overhead as possible. Especially when we know what's the max size and that we will not attempt to resize to larger than that...

Didn't touch that yet. Not sure if exchanging SETLENGTH with growable_resize will do the full trick since the function dispatch in frollapply is quite cumbersome and its hard to grasp whether we end up with a DT or a vector.

by.column=FALSE is when DT branches are in use, otherwise vector.
Note that functions there are very similar to each other, and may seem that could be merged into one by branching if (.) 1 else 2. It is kept like that to reduce the overhead. We want as few ifs as possible because those functions are called for every single observation of the input to forllapply().

@ben-schwen
Copy link
Member Author

by.column=FALSE is when DT branches are in use, otherwise vector. Note that functions there are very similar to each other, and may seem that could be merged into one by branching if (.) 1 else 2. It is kept like that to reduce the overhead. We want as few ifs as possible because those functions are called for every single observation of the input to forllapply().

You were right 👍. Exchanging SETLENGTH with growable_resize and patching your version of setgrowable using make_growable works!

All 16120 tests (last 7003.76) in tests/froll.Rraw completed ok in 00:01:39 elapsed (00:01:28 cpu)

jangorecki
jangorecki reviewed Sep 26, 2025
View reviewed changes
ben-schwen
ben-schwen commented Sep 26, 2025
View reviewed changes
ben-schwen
ben-schwen commented Sep 26, 2025
View reviewed changes
@rikivillalba rikivillalba mentioned this pull request Oct 9, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aitap aitap aitap left review comments

@jangorecki jangorecki jangorecki left review comments

@HughParsonage HughParsonage Awaiting requested review from HughParsonage HughParsonage is a code owner

@tdhock tdhock Awaiting requested review from tdhock

@MichaelChirico MichaelChirico Awaiting requested review from MichaelChirico MichaelChirico is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ben-schwen @jangorecki @aitap