-
Notifications
You must be signed in to change notification settings - Fork 917
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
Control pinned memory use with environment variables #17657
Merged
rapids-bot
merged 8 commits into
rapidsai:branch-25.02
from
vuule:fea-thresholds-env-vars
Jan 8, 2025
+4
−2
Merged
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
cfaa891
read env vars
vuule ca2b2cb
Merge branch 'branch-25.02' into fea-thresholds-env-vars
vuule 98fda91
copyright year
vuule 9e1ea65
Merge branch 'fea-thresholds-env-vars' of https://github.com/vuule/cu…
vuule 22654af
Merge branch 'branch-25.02' into fea-thresholds-env-vars
vuule b5c5068
Merge branch 'branch-25.02' into fea-thresholds-env-vars
vuule f62f1cb
Merge branch 'branch-25.02' into fea-thresholds-env-vars
vuule e7aea72
Merge branch 'branch-25.02' into fea-thresholds-env-vars
vuule File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
One issue with static variable is that, they are initialized only once. The user will not be able to change them after they are set. Can we remove static?
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.
I'm torn on this one. You're right, but:
I really like reading the env var once so I can log it cleanly (already a part of
getenv_or
).Also, there are (C++) APIs to set these thresholds at runtime.
I'd keep this static until we have a use case that requires runtime changes.
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.
Static or not, we want to cache this after reading it once. We shouldn't be querying the environment variable every time we need to decide how to pin/copy.
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.
I thought this as well and tried caching the large strings env var. Turns out repeated calls to getenv are very cheap. Not arguing for this option here, just a comment to make sure folks are aware of the actual performance.
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.
Ok cool, good to know!