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

tests: try to make s2n_mem_usage_test more useful #5139

Merged
merged 8 commits into from
Mar 5, 2025

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Feb 20, 2025

Release Summary:

Resolved issues:

resolves #5121 (comment)

Description of changes:

I always dread touching s2n_mem_usage because changing the expected values always breaks the test SOMEWHERE. I've also had the test fail locally even though I updated MEM_PER_CONNECTION to the value the test told me to use, just because of the complicated math. So this PR:

  1. Skips the test almost everywhere. Only enable the test in specific CI builds.
  2. Simplifies the math. I convert the measured vm memory to memory per connection, rather than trying to go the other direction.
  3. Allows less variance. We should really know if memory increases or decreases by more than 5%. That's not trivial.
  4. Don't rely on TEST_DEBUG_PRINT. We should just always print the output on failure.

Testing:

Example failure output from running it locally:

Actual KB per connection: 45
This is a -11.76% change

VmData initial:                  655360
VmData after allocations:      16117760
VmData after handshakes:       24354816
VmData after free handshake:   24354816
VmData after release:          24354816
Number of connections used:         252

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Feb 20, 2025
@lrstewart lrstewart force-pushed the mem_usage branch 10 times, most recently from ebd59f3 to 3fefd6e Compare February 20, 2025 22:40
@lrstewart lrstewart requested a review from goatgoose February 20, 2025 23:20
@lrstewart lrstewart marked this pull request as ready for review February 20, 2025 23:20
@lrstewart lrstewart requested a review from dougch as a code owner February 20, 2025 23:20
@@ -50,32 +50,11 @@
* usage. The greater the value, the more accurate the end result. */
#define MAX_CONNECTIONS 1000

/* This is roughly the current memory usage per connection, in KB */
Copy link
Contributor

Choose a reason for hiding this comment

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

good riddance to this bit.

Comment on lines 53 to 54
/* A change of more than 5% is significant */
#define ALLOWED_VARIANCE .05
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about it, should this be even stricter? 3%? Or should any change to the final "KB" number require an update to the test-- so 0%? That would still let changes <1KB through.

Copy link
Contributor

Choose a reason for hiding this comment

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

1KB seems like kind of a lot, right? I feel like 0% would be enough variance. Not sure though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's do 0 and we can always add some wiggle room later if we need it.

Comment on lines 53 to 54
/* A change of more than 5% is significant */
#define ALLOWED_VARIANCE .05
Copy link
Contributor

Choose a reason for hiding this comment

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

1KB seems like kind of a lot, right? I feel like 0% would be enough variance. Not sure though.

@lrstewart lrstewart requested a review from goatgoose February 27, 2025 01:15
Co-authored-by: Sam Clark <[email protected]>
@lrstewart lrstewart enabled auto-merge February 27, 2025 17:27
@lrstewart lrstewart added this pull request to the merge queue Mar 2, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 2, 2025
@lrstewart lrstewart added this pull request to the merge queue Mar 4, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 4, 2025
@lrstewart lrstewart enabled auto-merge March 4, 2025 19:53
Maybe I shouldn't keep ripping out old code
@lrstewart lrstewart added this pull request to the merge queue Mar 5, 2025
Merged via the queue into aws:main with commit 4ed4f1a Mar 5, 2025
46 checks passed
@lrstewart lrstewart deleted the mem_usage branch March 5, 2025 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants