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

Using my GitHub Action to observe the performance regression fixed by PR #4440 #8

Open
wants to merge 2 commits into
base: before-integrating-4440
Choose a base branch
from

Conversation

Anirban166
Copy link
Owner

This pull request mirrors #4440 which fixes the historical performance regression based on deep copy usage in indices for shallow() (as discovered in #4311) and includes two atime test cases - one to test for this (uses setindex()), and one to check for another unrelated regression test case.

This will trigger my GitHub Action, commenting a plot on this PR thread, along with the commit that generated it, the link to the artifact containing the test results, the elapsed time from the job's start to the completion of the installation steps, and the time it took to run all the tests. The comment will get updated on subsequent commits or pushes to this PR.

The plot is based on time and memory performance analysis using the atime package on different versions of data.table, and will have the two test cases each with seven different labels for points associated with commits:

  • HEAD (PR source or after-integrating-4440 here),
  • base (PR target or before-integrating-4440 here),
  • merge-base (common ancestor between base and head),
  • CRAN (latest version of data.table on the platform),
  • Regression (a commit that is affected by the performance regression),
  • Before (a commit from before the introduction of the performance regression),
  • Fixed (a commit where the performance regression has been fixed, should be the same or better as 'Before' in terms of performance)

Since this replicates the PR that fixes the regression for 'Test regression fixed in #​4440', I would expect the plot to show the commits associated with the HEAD, CRAN, Fixed, and Before labels to be fast and grouped together while base, merge-base, and Regression are slow.

For the other test case or 'Test regression fixed in #​5463', I would expect all the commits to be fast except the one associated with 'Regression'. That and 'Before' as it stands, should be separate (the rest have the same performance).

Note that the positions of 'Before', 'Regression', and 'Fixed' don't vary from PR to PR as they are fixed commit SHAs set manually.

To reproduce on another fork, one can use the git commands I used locally on my clone of this repository:

git checkout -b before-integrating-4440
git reset ad7b67c
git push --set-upstream origin before-integrating-4440
git switch -f master
git checkout -b after-integrating-4440
git reset 9d3b920
git push --set-upstream origin after-integrating-4440

Then place my workflow under .github/workflows for the before-integrating-4440 branch, and include the tests under inst/atime for the after-integrating-4440 branch like I did here.

Copy link

github-actions bot commented Apr 9, 2024

Comparison Plot

Generated via commit f057ee5

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

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

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

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.

2 participants