Skip to content

Added an atime test for performance improvement in forderv when reusing existing key and index attributes #6555

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

Merged
merged 15 commits into from
Oct 11, 2024
Merged

Added an atime test for performance improvement in forderv when reusing existing key and index attributes #6555

Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5a15dd2
Added the test with minor changes to comments
Anirban166 Oct 3, 2024
7e822ad
Narrow down on N
Anirban166 Oct 3, 2024
4ecb240
Go with default N spec
Anirban166 Oct 3, 2024
b0c10f5
Set seed for reproducibility given the use of sample()
Anirban166 Oct 3, 2024
ba72deb
rm sort reuse option
Anirban166 Oct 3, 2024
c519de1
for loop over retGrp=T,F
Oct 3, 2024
f24984a
historical sha1s from commits page instead of merge commit
Oct 3, 2024
6e69305
rm > from test name since that fails on CI
Oct 3, 2024
401f3d7
remove extra blank lines
Oct 3, 2024
7800a62
use setattr-> Fast always linear
Oct 3, 2024
7a16341
forderv retGrp=T or F, no setindex
tdhock Oct 8, 2024
6603d06
more detailed comment about expectations
tdhock Oct 8, 2024
6509080
restore old option after test
tdhock Oct 8, 2024
4e69115
log
tdhock Oct 8, 2024
13c4b1f
undo white space addition
tdhock Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion .ci/atime/tests.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,31 @@ for (extra.arg in extra.args.6107){
extra.test.list[[sprintf("fread(%s) improved in #6107", extra.arg)]] <- this.test
}

# Test case adapted from https://github.com/Rdatatable/data.table/pull/4386#issue-602528139 which is where the performance was improved.
for(retGrp_chr in c("T","F"))extra.test.list[[sprintf(
"forderv(retGrp=%s) improved in #4386", retGrp_chr
)]] <- list(
setup = quote({
dt <- data.table(group = rep(1:2, l=N))
}),
expr = substitute({
old.opt <- options(datatable.forder.auto.index = TRUE) # required for test, un-documented, comments in forder.c say it is for debugging only.
Copy link
Member Author

@Anirban166 Anirban166 Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Jan mentioned that it is only for 'debugging' since it was not exported (coming from his comment here), but we can probably ask him for more information if we want to document it or have more details (otherwise yes, not having this option set to TRUE will not show performance improvement)

data.table:::forderv(dt, "group", retGrp = RETGRP)
options(old.opt) # so the option does not affect other tests.
}, list(RETGRP=eval(str2lang(retGrp_chr)))),
## From ?bench::mark, "Each expression will always run at least twice,
## once to measure the memory allocation and store results
## and one or more times to measure timing."
## So for atime(times=10) that means 11 times total.
## First time for memory allocation measurement,
## (also sets the index of dt in this example),
## then 10 more times for time measurement.
## Timings should be constant if the cached index is used (Fast),
## and (log-)linear if the index is re-computed (Slow).
Slow = "b1b1832b0d2d4032b46477d9fe6efb15006664f4", # Parent of the first commit (https://github.com/Rdatatable/data.table/commit/b0efcf59442a7d086c6df17fa6a45c81b082322e) in the PR (https://github.com/Rdatatable/data.table/pull/4386/commits) where the performance was improved.
Fast = "ffe431fbc1fe2d52ed9499f78e7e16eae4d71a93" # Last commit of the PR (https://github.com/Rdatatable/data.table/pull/4386/commits) where the performance was improved.
)

# A list of performance tests.
#
# See documentation in https://github.com/Rdatatable/data.table/wiki/Performance-testing for best practices.
Expand Down Expand Up @@ -207,6 +232,6 @@ test.list <- atime::atime_test_list(
expr = data.table:::melt(DT, measure.vars = measure.vars),
Slow = "fd24a3105953f7785ea7414678ed8e04524e6955", # Parent of the merge commit (https://github.com/Rdatatable/data.table/commit/ed72e398df76a0fcfd134a4ad92356690e4210ea) of the PR (https://github.com/Rdatatable/data.table/pull/5054) that fixes the issue
Fast = "ed72e398df76a0fcfd134a4ad92356690e4210ea"), # Merge commit of the PR (https://github.com/Rdatatable/data.table/pull/5054) that fixes the issue

tests=extra.test.list)
# nolint end: undesirable_operator_linter.
Loading