-
Notifications
You must be signed in to change notification settings - Fork 8
perf: optimize EvaluatorBucketing #88
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
Open
yuzawa-san
wants to merge
3
commits into
launchdarkly:main
Choose a base branch
from
yuzawa-san:evaluator-bucketing-optimization
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
perf: optimize EvaluatorBucketing #88
yuzawa-san
wants to merge
3
commits into
launchdarkly:main
from
yuzawa-san:evaluator-bucketing-optimization
Conversation
This file contains hidden or 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
I have conducted some JMH microbenchmarking and found that 59247ee is not worthwhile. I believe the character by character conversion is not efficient relative, hence I have reverted that. There is a still an memory and throughput lift in my original commit (newVersionExtra vs oldVersion).
@Benchmark // 59247ee
public void newVersion(Blackhole blackhole) {
StringBuilder keyBuilder = new StringBuilder();
if (seed != null) {
keyBuilder.append(seed.intValue());
} else {
keyBuilder.append(flagOrSegmentKey).append('.').append(salt);
}
keyBuilder.append('.');
keyBuilder.append(USER_ID);
// turn the first 15 hex digits of this into a long
MessageDigest digest = DigestUtils.getSha1Digest();
digest.update(StandardCharsets.UTF_8.encode(CharBuffer.wrap(keyBuilder)));
byte[] hash = digest.digest();
long longVal = 0;
for (int i = 0; i < 7; i++) {
longVal <<= 8;
longVal |= (hash[i] & 0xff);
}
longVal <<= 4;
longVal |= ((hash[7] >> 4) & 0xf);
blackhole.consume(longVal);
}
@Benchmark // acd352d
public void newVersionExtra(Blackhole blackhole) {
StringBuilder keyBuilder = new StringBuilder();
if (seed != null) {
keyBuilder.append(seed.intValue());
} else {
keyBuilder.append(flagOrSegmentKey).append('.').append(salt);
}
keyBuilder.append('.');
keyBuilder.append(USER_ID);
// turn the first 15 hex digits of this into a long
byte[] hash = DigestUtils.sha1(keyBuilder.toString());
long longVal = 0;
for (int i = 0; i < 7; i++) {
longVal <<= 8;
longVal |= (hash[i] & 0xff);
}
longVal <<= 4;
longVal |= ((hash[7] >> 4) & 0xf);
blackhole.consume(longVal);
}
@Benchmark
public void oldVersion(Blackhole blackhole) {
String idHash = USER_ID;
String prefix;
if (seed != null) {
prefix = seed.toString();
} else {
prefix = flagOrSegmentKey + "." + salt;
}
String hash = DigestUtils.sha1Hex(prefix + "." + idHash).substring(0, 15);
long longVal = Long.parseLong(hash, 16);
blackhole.consume(longVal);
} |
Thank you for the contribution @yuzawa-san , I will try to take a look in the next few days. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Requirements
Related issues
n/a
Describe the solution you've provided
I have eliminated several intermediate allocations in this hot piece of code in my codebase. I used a single StringBuilder to accumulate the key to be hashed. The intermediate concatenations to make the key each had allocations. Additionally, I used StringBuilder.append(int) when possible to avoid the intermediate string conversion from int. I removed the hex and substring steps, both of which allocated strings and backing bytes. Instead, I did the equivalent calculation use bit operations on the underlying hash output bytes. I added a test to ensure the old logic and new logic are aligned.
Describe alternatives you've considered
Ideally, the StringBuilder would be able to go right from the appended Strings to utf8 bytes into the sha1, but sadly the StringBuilder creates an intermediate string that then gets converted into bytes for the digest. I do not believe I can make that better, but will sleep on it.nevermind, I found https://stackoverflow.com/questions/19472011/java-stringbuffer-to-byte-without-tostringnevermind to that, it would appear the copy is cheaper than the character conversion and wrapping
Additional context
This was the hotspot in our allocations flamegraph which I wish to improve.