-
Notifications
You must be signed in to change notification settings - Fork 990
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.
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
adding an atime test case Performance Regression with .N and := #PR5463 #6160
Conversation
Generated via commit adf09d8 Download link for the artifact containing the test results: ↓ atime-results.zip Time taken to finish the standard R installation steps: 13 minutes and 32 seconds Time taken to run |
.ci/atime/tests.R
Outdated
|
||
#issue reported in: https://github.com/Rdatatable/data.table/issues/5424 | ||
#Fixed in: https://github.com/Rdatatable/data.table/pull/5463 | ||
"Performance Regression with .N and := #PR5463" = atime::atime_test( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the reviews I received from @tdhock on the three existing test cases, the title would be of the form '<Function/Operator/Symbol> improved in #PRNumber' for performance improvement cases ('Fast' and 'Slow' labels) or '<Function/Operator/Symbol> regression fixed in #PRNumber' for performance regressions ('Before', 'Regression', and 'Fixed' labels)
(also the term 'Performance' is omitted from the titles since it's a given and would be repeated for each case if we were to use it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also please next time make your branch name more informative.
user-patch-number is a default from the github web interface right?
@Anirban166 can you please explain to @DorisAmoakohene about how to create branches with more informative names for PRs?
@tdhock I updated the new branch name with @Anirban166 and it closed this pull request because the old branch name got deleted and you can see the new pull request for this test here #6169 |
ok in the future try to make smaller branch names (if they are too large then the text is too small to read in the performance test plot for HEAD in the PR) |
Second that, also small thing but the title should be ' ... -regression-fixed-in-5463' (instead of 'adding-a-test-for-regression-for-5463') |
This atime test case discusses the issue of [Performance Regression with .N and := (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