Added a GitHub Action to check for performance regressions within the incoming changes from PRs, along with two test cases based on historical regressions

[Anirban166]

This adds a GHA that will get triggered by every commit in a pull request to the master branch, creating and updating one comment on the PR thread.

As discussed here, it aims to aid in detecting performance degradation for the code changes introduced in PRs using test cases based on historical regressions that we would be maintaining in inst/atime/tests.R (added the file for starters, containing the two cases that I used for the examples I mentioned there; more test cases are to be added in the future)

Should probably be skipped for certain PRs, like for changes not based on code (such as documentation or comments) or minor things that are known to not have an impact on performance.

Note: I couldn't come up with a good name for my workflow file so I just named it autocomment (hopefully not confusing since at present there isn't another GHA workflow being used in data.table that makes comments!), but please feel free to suggest a better name or change it later.

Closes #6065
Closes #4687

[github-actions]

Generated via commit a65b088

Download link for the artifact containing the test results: ↓ atime-results.zip

Time taken to finish the standard R installation steps: 11 minutes and 34 seconds

Time taken to run atime::atime_pkg on the tests: 3 minutes and 11 seconds

[tdhock]

thanks! can you please change the name of the file from autocomment to performance-tests?

[Anirban166]

thanks! can you please change the name of the file from autocomment to performance-tests?

Sure!

[MichaelChirico]

cases based on historical regressions that we would be maintaining in inst/atime/tests.R

Is there a particular reason we need to ship this with the package? My instinct would be to put it in e.g. the .ci folder.

I only ask because we already generate a CRAN NOTE based on the size of the package, a large part of which is coming from the huge text files in inst/tests. As the set of performance regression tests grows over time, it will only add more to this burden, but I don't see the benefit of every user having these tests available (whereas we do benefit from every user being able to report test.data.table() results).

[Anirban166]

@jangorecki Hello Jan!

I was wondering if you know of the commit/source that introduced the regression discussed in #4311 (since you mentioned in a comment there that the regression was already in 1.12.8)

[MichaelChirico]

Should probably be skipped for certain PRs, like for changes not based on code (such as documentation or comments) or minor things that are known to not have an impact on performance.

Yes, I'd be fine only acting on PRs that touch files in R/ or src/. That means a path filter.

[Anirban166]

Yes, I'd be fine only acting on PRs that touch files in R/ or src/. That means a path filter.

Right on!
I'll update my workflow with one shortly. (I've used it before in other projects, here for e.g.)

[Anirban166]

Is there a particular reason we need to ship this with the package? My instinct would be to put it in e.g. the .ci folder.

I read the manual and under 'Details' for the atime_pkg function (on page 5), it says:

There should be a file named pkg.path/inst/atime/tests.R which defines test.list, a list with names
corresponding to different tests.

So I presume yes, unfortunately. But I believe @tdhock can change this path to be customizable and then we can redirect?

As the set of performance regression tests grows over time, it will only add more to this burden, but I don't see the benefit of every user having these tests available (whereas we do benefit from every user being able to report test.data.table() results).

I completely agree.

[tdhock]

Is there a particular reason we need to ship this with the package? My instinct would be to put it in e.g. the .ci folder.

this is possible, see tdhock/atime#41 for an implementation, would that be OK?

[tdhock]

better comments, thanks! please revise again though.

[MichaelChirico]

this is possible, see tdhock/atime#41 for an implementation, would that be OK?

perfect, no need to address anything in this PR then

[MichaelChirico]

off-topic / thinking aloud: @tdhock have you considered extending {atime} for timing surfaces in 2 dimensions? i'm wondering if we'd benefit from (nrow, ncol) 2D benchmarking in some case. Now we are restricted to marginal approach (nrow, then ncol), but the interaction effects can sometimes be interesting.

[tdhock]

atime is inherently 1 dimensional (N varies), but you can make nrow and ncol vary with N

[MichaelChirico]

I am happy with the current state, I'll let Toby give final approval. Thanks again!

[tdhock]

thanks!

[tdhock]

@Anirban166 can you please make a follow-up PR which adds another test, and moves tests under .ci? (I should have asked you to do that before merging, but forgot)