Use `Random` through `ThreadLocal<Random>`

adinauer commented 3 weeks ago

:scroll: Description

Instead of using Random directly, we now go through SentryRandom.current() which keeps one instance per thread. While there's still some synchronization overhead when compared to ThreadLocalRandom it still performs much better than using Random from multiple threads directly.

NOTE: The value returned by SentryRandom.current() should not be referenced to avoid accessing the same instance across threads. Instead each usage should invoke SentryRandom.current() when using it.

:bulb: Motivation and Context

We've found reduced performance when using Random from multiple threads. While it's thead-safe, it slows down significantly. It'd be even better to use ThreadLocalRandom instead, since it has even better performance but we'd risk being flagged in security audits again (Random vs. SecureRandom).

See https://github.com/getsentry/sentry-java/pull/3818#issuecomment-2445034392 for more details on performance test results.

Vendoring ThreadLocalRandom (like we did with Random) doesn't seem possible as it's doing some advanced things like using Unsafe and AccessController.doPrivileged which causedjava.lang.IllegalAccessError: class io.sentry.util.ThreadLocalRandom (in unnamed module @0x58651fd0) cannot access class sun.security.action.GetPropertyAction (in module java.base) because module java.base does not export sun.security.action to unnamed module @0x58651fd0 as well as java.lang.NoClassDefFoundError: Could not initialize class io.sentry.util.ThreadLocalRandom when trying to vendor it.

:green_heart: How did you test it?

:pencil: 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.

:crystal_ball: Next steps

github-actions[bot] commented 3 weeks ago

	Fails
:no_entry_sign:	Please consider adding a changelog entry for the next release.

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

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Use `Random` through `ThreadLocal<Random>` ([#3835](https://github.com/getsentry/sentry-java/pull/3835))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by :no_entry_sign: dangerJS against 5863675e83d1c8dbc96dfba0ec5e5b741ba551bb

github-actions[bot] commented 3 weeks ago

Performance metrics :rocket:

	Plain	With Sentry	Diff
Startup time	423.02 ms	428.55 ms	5.53 ms
Size	1.70 MiB	2.35 MiB	667.91 KiB

Previous results on branch: feat/thread-local-random

Startup times

Revision	Plain	With Sentry	Diff
d1647d30bb653ccec620662f41b7ba48b20d00cd	363.80 ms	385.85 ms	22.06 ms

App size

Revision	Plain	With Sentry	Diff
d1647d30bb653ccec620662f41b7ba48b20d00cd	1.70 MiB	2.35 MiB	667.91 KiB

getsentry / sentry-java