Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

DorisAmoakohene
Copy link
Contributor

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

Copy link

github-actions bot commented May 29, 2024

Comparison Plot

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 atime::atime_pkg on the tests: 3 minutes and 45 seconds

.ci/atime/tests.R Show resolved Hide resolved

#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(
Copy link
Member

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)

.ci/atime/tests.R Show resolved Hide resolved
Copy link
Member

@tdhock tdhock left a 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?

.ci/atime/tests.R Show resolved Hide resolved
@DorisAmoakohene DorisAmoakohene deleted the DorisAmoakohene-patch-2 branch June 4, 2024 21:24
@DorisAmoakohene
Copy link
Contributor Author

@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

@tdhock
Copy link
Member

tdhock commented Jun 5, 2024

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)

@Anirban166
Copy link
Member

Second that, also small thing but the title should be ' ... -regression-fixed-in-5463' (instead of 'adding-a-test-for-regression-for-5463')

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants