adding an atime test case Performance Regression with .N and := #PR5463 #6169
Conversation
|
Generated via commit 515415e Download link for the artifact containing the test results: ↓ atime-results.zip Time taken to finish the standard R installation steps: 11 minutes and 13 seconds Time taken to run |
.ci/atime/tests.R
Outdated
| dt <- data.table( | ||
| id = seq_len(N), | ||
| val = rnorm(N)) | ||
| dt |
There was a problem hiding this comment.
please remove line with dt by itself which does nothing but is potentially confusing
|
great thanks |
|
please add your name to DESCRIPTION as a contributor |
.ci/atime/tests.R
Outdated
| val = rnorm(N)) | ||
| }, | ||
| expr = data.table:::`[.data.table`(dt, , .(vs = (sum(val))), by = .(id)), | ||
| Before = "be2f72e6f5c90622fe72e1c315ca05769a9dc854", # Parent of the regression causing commit (https://github.com/Rdatatable/data.table/commit/e793f53466d99f86e70fc2611b708ae8c601a451) in the PR that introduced the issue (https://github.com/Rdatatable/data.table/pull/4491/commits) |
There was a problem hiding this comment.
please use comment templates https://github.com/Rdatatable/data.table/wiki/Performance-testing#documentation-of-historical-commits
If this commit is in the PR then use
Parent of the commit which caused the regression in the PR (PR_LINK)"
otherwise use Parent of first commit (COMMIT_LINK) in the PR (PR_LINK) which causes the issue
| Before = "be2f72e6f5c90622fe72e1c315ca05769a9dc854", # Parent of the regression causing commit (https://github.com/Rdatatable/data.table/commit/e793f53466d99f86e70fc2611b708ae8c601a451) in the PR that introduced the issue (https://github.com/Rdatatable/data.table/pull/4491/commits) | ||
| Regression = "e793f53466d99f86e70fc2611b708ae8c601a451", # Commit responsible for regression in the PR that introduced the issue (https://github.com/Rdatatable/data.table/pull/4491/commits) | ||
| Fixed = "58409197426ced4714af842650b0cc3b9e2cb842") # Last commit in the PR that fixed the regression (https://github.com/Rdatatable/data.table/pull/5463/commits) | ||
| Before = "fda7fd9dbcac939df33769193eebd44731ee59b9", # Parent of the regression commit (https://github.com/Rdatatable/data.table/commit/c4a2085e35689a108d67dacb2f8261e4964d7e12) |
There was a problem hiding this comment.
parent of this Regression commit (c4a2085) is not appropriate here, because this Regression commit is the parent of the first commit in the fix PR (not the commit which caused the regression), so the parent of this Regression commit could still be slow.
If the previous Regression commit (e793f53) is the commit which caused the regression, then it would be appropriate to use its parent as Before.
Please revert this commit, and write proper documentation comments for the commit IDs you were using before.
|
closing since we already have a performance test for the issue which was fixed in #5463 |
This atime test case discusses the issue of [Performance Regression with .N and := (https://github.com/https://github.com/https://github.com//issues/5424)
The cause of the regression is related to the addition of the snprintf function in the assign.c. #4491
The Regression was fixed by creating targetDesc function and adding snprintf in assign.c Fixes Regression