perf: add experimental cityhash option for bandit shuffling - #394
Conversation
🦋 Changeset detectedLatest commit: 99f5019 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| // Check environment variable for experimental CityHash support | ||
| let bandit_hashing_algorithm = std::env::var("EPPO_EXPERIMENTAL_BANDITS_CITYHASH") | ||
| .ok() | ||
| .and_then(|val| { | ||
| if val == "1" || val == "true" || val == "TRUE" { | ||
| Some(BanditHashingAlgorithm::CityHash) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .unwrap_or(BanditHashingAlgorithm::Md5); |
There was a problem hiding this comment.
Don't want to read env variable on the hot evaluation path, so reading it here (which usually happens in background thread)
| subject_key, | ||
| subject_attributes, | ||
| actions.iter(), | ||
| configuration.bandit_hashing_algorithm, |
There was a problem hiding this comment.
^ new parameter (caused a bit of reformatting in this section)
| // Compute selection hash once | ||
| let selection_hash = { | ||
| let hash = base_ctx.clone().finish(); | ||
| hash as u32 as f64 / u32::MAX as f64 |
There was a problem hiding this comment.
"sharding" by 10k was arbitrary and somewhat useless, so using truncation to 32-bits in the new algorithm (which should be a tad faster and less biased)
831dd10 to
99f5019
Compare
| trait BanditHasher: Clone { | ||
| /// Create a new hasher pre-initialized with flag_key + "-" + subject_key | ||
| fn new(flag_key: &str, subject_key: &str) -> Self; | ||
|
|
||
| /// Get the selection hash (0.0..1.0) for choosing action based on weights | ||
| fn selection_hash(&self) -> f64; | ||
|
|
||
| /// Compute hash for shuffling a specific action | ||
| fn action_shuffle_hash(&self, action_key: &str) -> u64; | ||
| } |
There was a problem hiding this comment.
Nice job distilling the universal hashing operations needed for bandits here
| None | ||
| } | ||
| }) | ||
| .unwrap_or(BanditHashingAlgorithm::Md5); |
There was a problem hiding this comment.
Nice use of strategy pattern!
2 participants
Motivation and Context
Hashing (for shuffling actions) is the main contributor to bandit evaluation performance, and while we gained ~2x boost from various performance optimizations, there's only so much we can do without breaking compatibility with other SDKs.
This PR is what we can do if compatibility is not a concern. It adds a new experimental env variable to use CityHash instead of md5, boosting performance by another 2x+. Though this obviously produces incompatible results.
How has this been documented?
Changelog entry.
How has this been tested?
Benchmarks.