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

Add array/hash implicit allocation tests #10339

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

jeremyevans
Copy link
Contributor

These are designed to prevent allocation regressions (commits that increase the number of implicitly allocated arrays and hashes). We have already had three commits in the last couple weeks to fix allocation regressions:

This test suite should hopefully allow us to find such regressions in CI before commit, to avoid committing future allocation regressions.

This uses assert_separately to test each case, which is very slow. Hopefully there is a better way that can isolate the entire test suite, instead of isolating each individual check. This does need some isolation as it disables garbage collection to get accurate allocation counts, and it cannot be run concurrently with other threads in the same process.

@jeremyevans jeremyevans requested review from nobu and XrXr March 22, 2024 22:32
Copy link
Member

@XrXr XrXr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could group together the cases in each test method into one assert_separately if the current approach is too slow.

Thanks for putting these test in place!

Comment on lines 25 to 30
empty_array = empty_array = []
empty_hash = empty_hash = {}
array1 = array1 = [1]
hash1 = hash1 = {a: 2}
nill = nill = nil
block = block = lambda{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why these are assigned to twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid verbose warnings for the calls that do not use them.

@jeremyevans
Copy link
Contributor Author

You could group together the cases in each test method into one assert_separately if the current approach is too slow.

The main issue with that approach is that it stops at the first failure. I suppose I could work around that, building an array of all cases that fail, and then report it at the end if the array isn't empty.

One area this doesn't currently test is use of ruby2_keywords. I'd like to add tests for that, not only as a method modifier, but more importantly, when passing the flagged hash in a splat to all of the other method types.

These are designed to prevent allocation regressions (commits that
increase the number of implicitly allocated arrays and hashes). We
have already had three commits in the last couple weeks to fix
allocation regressions:

* 15dc3aa
* aceee71
* c388784

This test suite should hopefully allow us to find such regressions
in CI before commit, to avoid committing future allocation regressions.

This uses assert_separately around each set of tests.  Doing it for
each individual check was too slow.  Failures are gathered and
reported at the end of the the suite as a single assertion, with
the message describing all failures.
@jeremyevans
Copy link
Contributor Author

I pushed a rebased commit with a much faster approach, using one assert_separately per test. Assuming it passes CI, I plan to merge it. I'll try to add the ruby2_keywords tests later when I find time.

@jeremyevans jeremyevans merged commit e4d6479 into ruby:master Mar 27, 2024
98 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants