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.

Sign up for GitHub

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

Remove allocations from TimeBasedUUIDGenerator #104584

Merged
merged 1 commit into from
Jan 21, 2024
Merged

Remove allocations from TimeBasedUUIDGenerator #104584

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 12 additions & 16 deletions server/src/main/java/org/elasticsearch/common/TimeBasedUUIDGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ class TimeBasedUUIDGenerator implements UUIDGenerator {
assert SECURE_MUNGED_ADDRESS.length == 6;
}

private static final Base64.Encoder BASE_64_NO_PADDING = Base64.getUrlEncoder().withoutPadding();

// protected for testing
protected long currentTimeMillis() {
return System.currentTimeMillis();
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to your change: is there a reason this generator can't use the cached version of current time from ThreadPool?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it's a little bothersome code wise, because we use this thing as a static singleton and now it would have a non-static dependency on the threadpool.
Also, and I didn't do the math here, theoretically though probably hard in practice ... if someone sets a very long polling interval on the cache time, that would maybe introduce some chance of collision? Probably not still annoying to adjust this to depend on the pool :)

Expand All @@ -48,22 +50,16 @@ protected byte[] macAddress() {
@Override
public String getBase64UUID() {
final int sequenceId = sequenceNumber.incrementAndGet() & 0xffffff;
long currentTimeMillis = currentTimeMillis();

long timestamp = this.lastTimestamp.updateAndGet(lastTimestamp -> {
// Don't let timestamp go backwards, at least "on our watch" (while this JVM is running). We are
// still vulnerable if we are shut down, clock goes backwards, and we restart... for this we
// randomize the sequenceNumber on init to decrease chance of collision:
long nonBackwardsTimestamp = Math.max(lastTimestamp, currentTimeMillis);

if (sequenceId == 0) {
// Always force the clock to increment whenever sequence number is 0, in case we have a long
// time-slip backwards:
nonBackwardsTimestamp++;
}

return nonBackwardsTimestamp;
});
// Don't let timestamp go backwards, at least "on our watch" (while this JVM is running). We are
// still vulnerable if we are shut down, clock goes backwards, and we restart... for this we
// randomize the sequenceNumber on init to decrease chance of collision:
long timestamp = this.lastTimestamp.accumulateAndGet(
currentTimeMillis(),
// Always force the clock to increment whenever sequence number is 0, in case we have a long
// time-slip backwards:
sequenceId == 0 ? (lastTimestamp, currentTimeMillis) -> Math.max(lastTimestamp, currentTimeMillis) + 1 : Math::max
);

final byte[] uuidBytes = new byte[15];
int i = 0;
Expand Down Expand Up @@ -106,6 +102,6 @@ public String getBase64UUID() {

assert i == uuidBytes.length;

return Base64.getUrlEncoder().withoutPadding().encodeToString(uuidBytes);
return BASE_64_NO_PADDING.encodeToString(uuidBytes);
}
}