Faster ID generation #3818

lbloder · 2024-10-23T14:22:35Z

📜 Description

As proposed in #3809, this PR implements a faster way to generate IDs used in Sentry.
In addition to the proposal two methods were added to directly generate an ID String without the need to call two methods:

SentryUUID.generateSentryId()
SentryUUID.generateSpanId()

SentryUUID.generateSpanId() only generates the first part of a UUID as before we would create a full uuid and then only use the first 16 out of 32 characters. Therefore we only need to create 8 random bytes instead of 16.

💡 Motivation and Context

Implements #3809

💚 How did you test it?

📝 Checklist

I reviewed the submitted code.
I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

# Conflicts: # CHANGELOG.md

Co-authored-by: Stefano <[email protected]>

…e difference of .equals if objects are not the same

…va into feat/lazy-span-id-v8

github-actions · 2024-10-23T14:23:00Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 5c2c580

romtsn · 2024-10-23T15:57:09Z

sentry/src/main/java/io/sentry/SentryUUID.java

+
+/**
+ * Utility class for faster id generation for SentryId, SpanId, and unique filenames uses throughout
+ * the SDK It uses our vendored Random class instead of SecureRandom for improved performance It


But it doesn't 😅

not yet :)
Sorry, wanted to add a comment here in the PR, that we need to first merge the main branch to 8.x.x and then to this one

romtsn · 2024-10-23T15:57:20Z

sentry/src/main/java/io/sentry/SentryUUID.java

+package io.sentry;
+
+import java.util.Arrays;
+import java.util.Random;


Suggested change

import java.util.Random;

import io.sentry.util.Random;

lbloder · 2024-10-23T20:43:38Z

sentry/src/main/java/io/sentry/SentryUUID.java

+   * @throws IllegalArgumentException if the given character sequence does not conform to the string
+   *     representation as described in {@link UUID#toString()}
+   */
+  public static UUID parseUUID(final CharSequence uuidSequence) {


Not sure if we even need this

I presume no, as long as we use String values in the SDK.

lbloder · 2024-10-23T20:47:32Z

sentry/src/main/java/io/sentry/SentryUUID.java

+    randomBytes[6] &= 0x0f; /* clear version        */
+    randomBytes[6] |= 0x40; /* set to version 4     */
+    randomBytes[8] &= 0x3f; /* clear variant        */
+    randomBytes[8] |= (byte) 0x80; /* set to IETF variant  */


This method is taken from java.uitl.UUID.
Do we need this part? Do we want/need to add those flags to our random ids?

Trying to clarify internally. Will update here when I get an answer.

We should keep it in. Also see https://develop.sentry.dev/sdk/data-model/event-payloads/#required-attributes

There's some validations for IDs, e.g.:

https://github.com/getsentry/relay/blob/c0756f24845bbf5b2bcefa537cefc295abc46d73/relay-event-schema/src/protocol/contexts/trace.rs#L66-L67

https://github.com/getsentry/relay/blob/c0756f24845bbf5b2bcefa537cefc295abc46d73/relay-event-schema/src/protocol/contexts/trace.rs#L17-L18

Let's verify those are compatible.

It's expecting a 16/32 char hexstring that isn't all zeroes. To avoid this even in case random generates an all 0 ID, we should keep the version / variant bits set.

lbloder · 2024-10-24T06:28:55Z

sentry/src/main/java/io/sentry/SentryUUID.java

+   * @param uuid the UUID to represent as a string
+   * @return a string representation of the given UUID
+   */
+  public static String toString(final UUID uuid) {


This returns an acutal UUID String including -. Do we need this for our purposes?

We should be able to replace all UUID usages that have - with simply using the String value without - as it should mostly be for our own IDs and some unique paths.

Great, will remove this method then.

lbloder · 2024-10-24T07:45:44Z

In addition to the proposal in #3809, which focused on the generation of a 32 char SentryId, this PR also aims to further improve the generation of SpanId which is only 16 char in length.
Previously a SpanId was generated like this UUID.randomUUID().toString().replace("-", "").substring(0, 16).

Therefore there is even more room for optimization here, as we do not actually need to generate a full UUID, e.g. 16 random bytes, but only 8 random bytes as only half of the resulting String is being used. Also, when generating only the needed 8 bytes the substring part can also be eliminated.

I did some benchmarks on this with the same parameters as the benchmarks in #3809 comparing the original way of generating a SpanId with the changes of this PR:

Java 17

Benchmark	Score		Error
generateSpanIdSecureRandomToStringReplaceAndSubstring16	2357463,435	±	11044,652	ops/s	100%
generateSpanIdRandomFastUUIDString	28488104,485	±	236823,660	ops/s	1208%

Java 8

Benchmark	Score		Error
generateSpanIdSecureRandomToStringReplaceAndSubstring16	1017070,653	±	5762,135	ops/s	100%
generateSpanIdRandomFastUUIDString	28461132,111	±	122920,961	ops/s	2798%

adinauer · 2024-10-29T06:44:45Z

sentry/src/main/java/io/sentry/SentryUUID.java

+   * @param uuid the UUID to represent as a string
+   * @return a string representation of the given UUID
+   */
+  public static String toString(final UUID uuid) {


We should be able to replace all UUID usages that have - with simply using the String value without - as it should mostly be for our own IDs and some unique paths.

adinauer · 2024-10-29T06:50:03Z

sentry/src/main/java/io/sentry/SentryUUID.java

+    randomBytes[6] &= 0x0f; /* clear version        */
+    randomBytes[6] |= 0x40; /* set to version 4     */
+    randomBytes[8] &= 0x3f; /* clear variant        */
+    randomBytes[8] |= (byte) 0x80; /* set to IETF variant  */


Trying to clarify internally. Will update here when I get an answer.

adinauer · 2024-10-29T06:54:34Z

sentry/src/main/java/io/sentry/SentryUUID.java

+    randomBytes[6] &= 0x0f; /* clear version        */
+    randomBytes[6] |= 0x40; /* set to version 4     */
+    randomBytes[8] &= 0x3f; /* clear variant        */
+    randomBytes[8] |= (byte) 0x80; /* set to IETF variant  */


We should keep it in. Also see https://develop.sentry.dev/sdk/data-model/event-payloads/#required-attributes

There's some validations for IDs, e.g.:

https://github.com/getsentry/relay/blob/c0756f24845bbf5b2bcefa537cefc295abc46d73/relay-event-schema/src/protocol/contexts/trace.rs#L66-L67

https://github.com/getsentry/relay/blob/c0756f24845bbf5b2bcefa537cefc295abc46d73/relay-event-schema/src/protocol/contexts/trace.rs#L17-L18

Let's verify those are compatible.

It's expecting a 16/32 char hexstring that isn't all zeroes. To avoid this even in case random generates an all 0 ID, we should keep the version / variant bits set.

adinauer · 2024-10-29T07:13:41Z

sentry/src/main/java/io/sentry/SentryUUID.java

+  }
+
+  private static class Holder {
+    static final Random numberGenerator = new Random();


According to https://docs.oracle.com/javase/7/docs/api/java/util/Random.html there might be poor performance when using Random from multiple threads.

Instances of java.util.Random are threadsafe. However, the concurrent use of the same java.util.Random instance across threads may encounter contention and consequent poor performance. Consider instead using ThreadLocalRandom in multithreaded designs.

Should we vendor ThreadLocalRandom as well and use that?

Did you also do (performance) testing in a multi threaded environment?

Hmm, good point. No, didn't do multithreaded perf testing. could try to spin up something to compare Random to ThreadLocalRandom

Yes, this does make quite a difference. Below is a comparison for UUID generation:

Benchmark Score Error Units Threads Comparison

uuidGenerationRandom 23789834,814 ± 117697,18 ops/s 1 100%

uuidGenerationRandomMultiThreaded 2280766,845 ± 263522,36 ops/s 10 10%

uuidGenerationThreadLocalRandomMultiThreaded 185639075,638 ± 5090316,9 ops/s 10 780%

whew, what's the baseline we're comparing to here? are all of those using the FastUUID code just with a different Random implementation?

The code from FastUUID is only responsible for converting the UUID to string. This was not done in this test.

I tested the SentryUUID.randomUUID() function, which is a clone of the java.util.UUID.randomUUID() function with java.util.Random as the random generator.

So, Results 1 and 2 test the above mentioned function in single-threaded vs multi-threaded scenarios.

Result 3 tests the same function with java.util.Random replaced with java.util.concurrent.ThreadLocalRandom. I replaced Random random = SentryUUID.Holder.numberGenerator; with Random random = ThreadLocalRandom.current();

Also, the benchmark code can now be found here: https://github.com/lbloder/sentry-uuid-bench

adinauer · 2024-10-29T07:14:33Z

sentry/src/main/java/io/sentry/SentryUUID.java

+   * @throws IllegalArgumentException if the given character sequence does not conform to the string
+   *     representation as described in {@link UUID#toString()}
+   */
+  public static UUID parseUUID(final CharSequence uuidSequence) {


I presume no, as long as we use String values in the SDK.

github-actions · 2024-10-29T14:24:53Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	449.27 ms	481.96 ms	32.69 ms
Size	1.65 MiB	2.31 MiB	672.61 KiB

Previous results on branch: feat/fast-id-generation

Startup times

Revision	Plain	With Sentry	Diff
`d909f73`	455.98 ms	472.71 ms	16.74 ms
`6449d66`	435.18 ms	477.48 ms	42.30 ms
`c46ade5`	406.26 ms	446.62 ms	40.36 ms
`470431c`	431.02 ms	469.31 ms	38.29 ms
`70c2f67`	426.29 ms	460.26 ms	33.97 ms

App size

Revision	Plain	With Sentry	Diff
`d909f73`	1.65 MiB	2.30 MiB	672.27 KiB
`6449d66`	1.70 MiB	2.36 MiB	671.75 KiB
`c46ade5`	1.65 MiB	2.30 MiB	672.27 KiB
`470431c`	1.70 MiB	2.36 MiB	671.77 KiB
`70c2f67`	1.70 MiB	2.36 MiB	671.63 KiB

lbloder · 2024-10-29T18:25:22Z

Did some additional Testing for MultiThreading based on this comment from @adinauer.

The results below show Test runs for:

java.util.Random single-threaded
java.util.random multi-threaded with 10 Threads
java.util.concurrent.ThreadLocalRandom with 10 Threads
A version in which we manage a ThreadLocal<java.util.Random> ourselves

Results

Benchmark	Score		Error	Units	Threads	Comparison
uuidGenerationRandom	23829634,539	±	101112,796	ops/s	1	100%
uuidGenerationRandomMultiThreaded	2458588,180	±	339397,214	ops/s	10	10%
uuidGenerationThreadLocalRandomMultiThreaded	180884073,702	±	2482343,414	ops/s	10	759%	157%
uuidGenerationThreadLocalRandomMultiThreadedSelfManaged	114937169,398	±	1555655,255	ops/s	10	482%	100%

We can see, that ThreadLocalRandom is highly optimized, outpacing our self-managed version by ~50%. However, we still get an almost 5x increase compared to using a single Random instance.

Secure Random

Out of curiosity, I also did a benchmark run to compare UUID.randomUUID in single-threaded and multi-threaded scenarios:

Benchmark	Score		Error	Units	Threads	Comparison
uuidGenerationSecureRandom	3229714,852	±	13916,035	ops/s	1	100%
uuidGenerationSecureRandomMultiThreaded	1567325,134	±	36688,679	ops/s	10	49%

We can see, that SecureRandom performance is also impacted when running in multiple threads, however, not nearly as much as Random.

# Conflicts: # CHANGELOG.md # sentry/src/main/java/io/sentry/SpanId.java # sentry/src/main/java/io/sentry/protocol/SentryId.java # sentry/src/test/java/io/sentry/protocol/SentryIdTest.kt

…ce when invoking with string

…-java into feat/fast-id-generation

…to SentryId constructor

lbloder and others added 19 commits October 7, 2024 10:52

lazy uuid generation for SentryId and SpanId

22029c1

add changelog

f947f82

Merge branch '8.x.x' into feat/lazy-span-id-v8

debd140

add tests for lazy init, rework SentryId to cache string result

e6df0ad

Merge branch '8.x.x' into feat/lazy-span-id-v8

8a93fec

Merge branch '8.x.x' into feat/lazy-span-id-v8

fe8ccb0

# Conflicts: # CHANGELOG.md

Merge branch '8.x.x' into feat/lazy-span-id-v8

28b5bde

fix changelog

f2fba85

Merge branch '8.x.x' into feat/lazy-span-id-v8

3e1a32c

# Conflicts: # CHANGELOG.md

Update sentry/src/main/java/io/sentry/protocol/SentryId.java

c7ab294

Co-authored-by: Stefano <[email protected]>

Update sentry/src/main/java/io/sentry/protocol/SentryId.java

f1e66b2

Co-authored-by: Stefano <[email protected]>

Update sentry/src/main/java/io/sentry/protocol/SentryId.java

76b23c8

Co-authored-by: Stefano <[email protected]>

Update sentry/src/main/java/io/sentry/protocol/SentryId.java

8599535

Co-authored-by: Stefano <[email protected]>

Update sentry/src/main/java/io/sentry/protocol/SentryId.java

3bae1d7

Co-authored-by: Stefano <[email protected]>

Format code

edba0c7

Add object comparison to SpanFrameMetricsCollector only check for tim…

c18a74d

…e difference of .equals if objects are not the same

Merge branch 'feat/lazy-span-id-v8' of github.com:getsentry/sentry-ja…

35b17db

…va into feat/lazy-span-id-v8

add proposed SentryUUID class

a50d881

[WIP] use SentryUUID instead of UUID.random().toString

149d59c

Format code

6418a33

romtsn reviewed Oct 23, 2024

View reviewed changes

lbloder commented Oct 23, 2024

View reviewed changes

lbloder commented Oct 24, 2024

View reviewed changes

adinauer reviewed Oct 29, 2024

View reviewed changes

split SentryUUID internals into separate classes for easier attribution

5314e83

adinauer approved these changes Oct 29, 2024

View reviewed changes

Merge branch '8.x.x' into feat/lazy-span-id-v8

58a00c4

lbloder marked this pull request as ready for review October 29, 2024 14:21

lbloder requested review from stefanosiano and markushi as code owners October 29, 2024 14:21

lbloder added 2 commits October 29, 2024 15:25

add changelog

097738b

Merge branch 'feat/lazy-span-id-v8' into feat/fast-id-generation

3518fce

lbloder mentioned this pull request Oct 29, 2024

SessionID as string #3834

Merged

7 tasks

Base automatically changed from feat/lazy-span-id-v8 to 8.x.x October 29, 2024 14:58

An error occurred while trying to automatically change base from feat/lazy-span-id-v8 to 8.x.x October 29, 2024 14:58

fix tests

d11c3cc

lbloder added 2 commits October 29, 2024 19:31

Merge branch '8.x.x' into feat/fast-id-generation

1ea4212

# Conflicts: # CHANGELOG.md # sentry/src/main/java/io/sentry/SpanId.java # sentry/src/main/java/io/sentry/protocol/SentryId.java # sentry/src/test/java/io/sentry/protocol/SentryIdTest.kt

fix SpanId.EMPTY_ID

81a3bdb

adinauer mentioned this pull request Oct 30, 2024

Use Random through ThreadLocal<Random> #3835

Merged

7 tasks

lbloder and others added 9 commits November 4, 2024 10:52

Merge branch '8.x.x' into feat/fast-id-generation

5d00ae5

test normalized is never called in no-arg constructor, only called on…

fac412e

…ce when invoking with string

Merge branch '8.x.x' into feat/fast-id-generation

9f360d1

fix test

30b2602

Format code

05540a3

use new Random Generator

143c26e

Merge branch 'feat/fast-id-generation' of github.com:getsentry/sentry…

57d0a9b

…-java into feat/fast-id-generation

fix Sentry Empty ID, replace dashes if 36 char uuid String is passed …

45c3cbe

…to SentryId constructor

fix Tests for SpanId, add tests for SentryUUID and UUIDStringUtils

5c2c580

adinauer merged commit 741807a into 8.x.x Nov 11, 2024
30 checks passed

adinauer deleted the feat/fast-id-generation branch November 11, 2024 04:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster ID generation #3818

Faster ID generation #3818

lbloder commented Oct 23, 2024

github-actions bot commented Oct 23, 2024 •

edited

Loading

romtsn Oct 23, 2024

lbloder Oct 23, 2024

romtsn Oct 23, 2024

lbloder Oct 23, 2024

adinauer Oct 29, 2024

lbloder Oct 23, 2024

adinauer Oct 29, 2024

adinauer Oct 29, 2024

lbloder Oct 24, 2024

adinauer Oct 29, 2024

lbloder Oct 29, 2024

lbloder commented Oct 24, 2024

adinauer Oct 29, 2024

adinauer Oct 29, 2024

adinauer Oct 29, 2024

adinauer Oct 29, 2024

lbloder Oct 29, 2024

lbloder Oct 29, 2024

adinauer Oct 29, 2024

lbloder Oct 29, 2024

lbloder Oct 29, 2024 •

edited

Loading

adinauer Oct 29, 2024

github-actions bot commented Oct 29, 2024 •

edited

Loading

Previous results on branch: feat/fast-id-generation

Startup times

App size

lbloder commented Oct 29, 2024

Benchmark	Score		Error	Units	Threads	Comparison
uuidGenerationRandom	23789834,814	±	117697,18	ops/s	1	100%
uuidGenerationRandomMultiThreaded	2280766,845	±	263522,36	ops/s	10	10%
uuidGenerationThreadLocalRandomMultiThreaded	185639075,638	±	5090316,9	ops/s	10	780%

Faster ID generation #3818

Faster ID generation #3818

Conversation

lbloder commented Oct 23, 2024

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbloder commented Oct 24, 2024

Java 17

Java 8

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbloder Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 29, 2024 • edited Loading

Performance metrics 🚀

Previous results on branch: feat/fast-id-generation

Startup times

App size

lbloder commented Oct 29, 2024

Results

Secure Random

github-actions bot commented Oct 23, 2024 •

edited

Loading

lbloder Oct 29, 2024 •

edited

Loading

github-actions bot commented Oct 29, 2024 •

edited

Loading