Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement MsQuicConfiguration cache #99371
Implement MsQuicConfiguration cache #99371
Changes from 4 commits
8b81795
6f94f38
34c314d
6825c3a
b5689a5
40c4bc2
82965d2
0578632
8e26582
c1ddffb
ebab536
3e82d24
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may lead to frequently throwing out and recreating entries once you have > 32 certs in use.
E.g. you're talking to 40 different hosts, and every time you reach 32 most of these objects will be idle so they're all thrown out, and you start from the beginning.
It may be worth adding slightly more logic here (e.g. skip throwing away entries that were used in the last second / use a timer instead of checking
Count
/ allow only oneCleanupCache
call per X amount of time ...)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The strategy has been copied from the one we do in SslStream for Windows, I am not aware of any reported issues of too frequent cleanup (but maybe it just was not used for this sort of thing, cc @wfurt) and the code over there is ancient. How common is such a scenario?
My thoughts on this is to keep it simple, we are not trying to prevent all unnecessary allocations, only the frequent ones, if, say, app rotates 40 different configurations but does outbound connection once every second (i.e. 40s to rotate through all of them), then I don't think creating a fresh configuration makes a measurable dent in CPU usage.
If the app makes very frequent connections to multiple hosts and we should care about not doing extra work, then most of the configurations would still be in use and once there is a burst of connections and we stay above 32 connections, then there is no attempt for cleanup until the cache goes to 64 items, and so on.
I am not opposed to adding additional conditions for the cleanup, but adding additional conditions like "only 1 cleanup per X seconds" feels arbitrary without a more concrete evidence that it will help.
Another option would be making the cache cleanup size configurable via envvar so that we have a way out if some customer hits the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, this seems sufficient atm and if this proves problematic, we can always revisit the logic here.