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.

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

Make UUIDGen more performant #3647

Merged
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
dbd35db
Wip(2882): Make UUIDGEN more performant
diogocanut May 24, 2023
22db811
draf: porting java randomUUID
diogocanut May 25, 2023
2202a9f
wip
diogocanut May 25, 2023
cb28120
new ScalaRandom
diogocanut May 25, 2023
c7a167b
Move javaSecuritySecureRandom to SecureRandomCompanionPlatform
diogocanut May 26, 2023
77d6081
share unsafe UUID builder logic between js and jvm
diogocanut May 29, 2023
6552d20
remove UUIDGenCompanionPlatformLowPriority file
diogocanut May 29, 2023
ab1573d
remove unused import
diogocanut May 29, 2023
3d0d4c4
move fromSecureRandom to UUIDGen
diogocanut May 30, 2023
b0bf31c
avoid duplication of UUIDGen code
diogocanut May 30, 2023
47b3544
fix: implicit definition needs to be given explicitly
diogocanut May 30, 2023
d906316
keep javaSecuritySecureRandom Scaladoc
diogocanut Jun 1, 2023
b611dda
Use System.getProperty instead of sys.props.get
diogocanut Jun 1, 2023
1655b79
gitignore direnv comment
diogocanut Jun 1, 2023
5a9259c
remove implicit secureRandom def and deprecate old fromSync methods
diogocanut Jun 2, 2023
1c6b6ec
add unsafeJavaSecuritySecureRandom
diogocanut Jun 3, 2023
c4c6dc0
provide Sync.type as constructor param to ScalaRandom
diogocanut Jun 3, 2023
e98eb09
add implicit SecureRandom for IO
diogocanut Jun 4, 2023
c2633ee
fix: failing CI
diogocanut Jun 5, 2023
816d1c4
review misc changes
diogocanut Jun 5, 2023
5519064
review: remove doc and case Throwable
diogocanut Jun 6, 2023
db8a428
improve getAndUpdate expression
diogocanut Jun 7, 2023
201d8e0
using unsafeJavaSecuritySecureRandom in UUIDGenCompanionPlatformLowPr…
diogocanut Jun 7, 2023
4b4ded3
Merge branch 'typelevel:series/3.x' into 2882/make-UUIDGen-more-perfo…
diogocanut Jun 7, 2023
eead224
Add ported from http4s comment
diogocanut Jun 8, 2023
7764e24
Fix unused import
armanbilge Nov 21, 2024
31897c4
Merge remote-tracking branch 'upstream/series/3.x' into 2882/make-UUI…
armanbilge Nov 21, 2024
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
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,6 @@ metals.sbt

# npm
node_modules/

# nix flake
.direnv/
diogocanut marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 4 additions & 1 deletion core/shared/src/main/scala/cats/effect/IO.scala
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import cats.data.Ior
import cats.effect.instances.spawn
import cats.effect.kernel.CancelScope
import cats.effect.kernel.GenTemporal.handleDuration
import cats.effect.std.{Backpressure, Console, Env, Supervisor, UUIDGen}
import cats.effect.std.{Backpressure, Console, Env, SecureRandom, Supervisor, UUIDGen}
import cats.effect.tracing.{Tracing, TracingEvent}
import cats.syntax.all._

Expand Down Expand Up @@ -1963,6 +1963,9 @@ object IO extends IOCompanionPlatform with IOLowPriorityImplicits {
// later in the file
private[this] val _never: IO[Nothing] = asyncForIO.never

implicit def secureRandom: SecureRandom[IO] =
Copy link
Member

Choose a reason for hiding this comment

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

Otherwise it will create a new one each time :)

Suggested change
implicit def secureRandom: SecureRandom[IO] =
lazy val secureRandom: SecureRandom[IO] =

Call-out for other reviewers: are we okay about unsafely creating a global secure random like this? I think having this would greatly enhance UX, but it creates a muddy precedent to use a pattern that has gone wrong sometimes e.g. FS2 Network ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, how to add the implicit to scope having it as lazy val?

[warn] /Users/diogocanut/Projects/cats-effect/core/shared/src/main/scala/cats/effect/IO.scala:1447:37: method fromSync in trait UUIDGenCompanionPlatformLowPriority is deprecated (since 3.6.0): Put an implicit `SecureRandom.javaSecuritySecureRandom` into scope to get a more efficient `UUIDGen`, or directly call `UUIDGen.fromSecureRandom`
[warn]   def randomUUID: IO[UUID] = UUIDGen[IO].randomUUID

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry, I meant to write implicit lazy val :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@armanbilge What was the problem in FS2 Network?

Copy link
Member

Choose a reason for hiding this comment

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

Well, it's a rather involved discussion :) so the problem is that FS2 promises an implicit Network[IO], but this is technically encapsulating some global resources (namely, I/O threads). This was working well enough, until we started the polling system work. It created a significant challenge, since now we have to thread the IOApps polling system into this globally promised Network[IO].

If instead FS2 had been offering a Resource[IO, Network[IO]] that you had to construct explicitly, it would have been a lot easier to work with.

The fundamental question here is, when is it okay to "cheat" and unsafely allocate global shared state or resources, instead of forcing it to be explicitly allocated in IO or Resource by the user. It's some kind of UX trade-off.

See a similar place where we faced this in:

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I think I understand. Btw, I'm not convinced a Random[IO] must be implicitly available: there isn't "one obvious preferable" Random[IO]. However, for SecureRandom, there could be: in fact, this PR tries to do that... so I think I'm okay with it in this specific case. (But it's possible i just don't appreciate the possible future pain this could cause :-)

SecureRandom.unsafeJavaSecuritySecureRandom[IO]

// implementations

private[effect] final case class Pure[+A](value: A) extends IO[A] {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,35 +28,24 @@

package cats.effect.std

import cats.Applicative
import cats.effect.kernel.Sync
import cats.effect.std.Random.ScalaRandom

import java.util.UUID

private[std] trait UUIDGenCompanionPlatform {
implicit def fromSync[F[_]](implicit ev: Sync[F]): UUIDGen[F] = new UUIDGen[F] {
private val csprng = new SecureRandom.JavaSecureRandom()
private val randomUUIDBuffer = new Array[Byte](16)
override final val randomUUID: F[UUID] =
ev.delay {
val buffer = randomUUIDBuffer // local copy

/* We use nextBytes() because that is the primitive of most secure RNGs,
* and therefore it allows to perform a unique call to the underlying
* secure RNG.
*/
csprng.nextBytes(randomUUIDBuffer)

@inline def intFromBuffer(i: Int): Int =
(buffer(i) << 24) | ((buffer(i + 1) & 0xff) << 16) | ((buffer(
i + 2) & 0xff) << 8) | (buffer(i + 3) & 0xff)

val i1 = intFromBuffer(0)
val i2 = (intFromBuffer(4) & ~0x0000f000) | 0x00004000
val i3 = (intFromBuffer(8) & ~0xc0000000) | 0x80000000
val i4 = intFromBuffer(12)
val msb = (i1.toLong << 32) | (i2.toLong & 0xffffffffL)
val lsb = (i3.toLong << 32) | (i4.toLong & 0xffffffffL)
new UUID(msb, lsb)
}
private[std] trait UUIDGenCompanionPlatform extends UUIDGenCompanionPlatformLowPriority
diogocanut marked this conversation as resolved.
Show resolved Hide resolved

private[std] trait UUIDGenCompanionPlatformLowPriority {

private[this] def secureRandom[F[_]](implicit ev: Sync[F]): SecureRandom[F] =
diogocanut marked this conversation as resolved.
Show resolved Hide resolved
new ScalaRandom[F](Applicative[F].pure(new SecureRandom.JavaSecureRandom()))
with SecureRandom[F] {}

@deprecated(
"Put an implicit `SecureRandom.javaSecuritySecureRandom` into scope to get a more efficient `UUIDGen`, or directly call `UUIDGen.fromSecureRandom`",
"3.6.0"
)
implicit def fromSync[F[_]](implicit ev: Sync[F]): UUIDGen[F] = {
UUIDGen.fromSecureRandom[F](ev, secureRandom[F])
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@

package cats.effect.std

import cats.Applicative
import cats.effect.kernel.Sync
import cats.effect.std.Random.ScalaRandom

import scala.scalajs.js
import scala.scalajs.js.typedarray._

Expand Down Expand Up @@ -110,4 +114,10 @@ private[std] trait SecureRandomCompanionPlatform {
}
}

def javaSecuritySecureRandom[F[_]: Sync]: F[SecureRandom[F]] =
Sync[F].delay(unsafeJavaSecuritySecureRandom)

private[effect] def unsafeJavaSecuritySecureRandom[F[_]: Sync]: SecureRandom[F] =
diogocanut marked this conversation as resolved.
Show resolved Hide resolved
new ScalaRandom[F](Applicative[F].pure(new JavaSecureRandom())) with SecureRandom[F] {}

}
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,120 @@
* limitations under the License.
*/

package cats.effect.std
package cats
package effect
package std

import cats._
import cats.effect.kernel._
import cats.effect.std.Random.ScalaRandom

import scala.util.{Random => SRandom, Try}

import java.util.concurrent.atomic.AtomicInteger

private[std] trait SecureRandomCompanionPlatform {
private[std] type JavaSecureRandom = java.security.SecureRandom
private[std] def getInstance: JavaSecureRandom =
diogocanut marked this conversation as resolved.
Show resolved Hide resolved
java.security.SecureRandom.getInstance("NativePRNGNonBlocking")

private def javaUtilRandom[F[_]: Sync](random: JavaSecureRandom): SecureRandom[F] =
new ScalaRandom[F](Applicative[F].pure(random)) with SecureRandom[F] {}

/**
* Creates a blocking Random instance.
*
* @param random
* a potentially blocking instance of java.util.Random
*/
def javaUtilRandomBlocking[F[_]: Sync](random: JavaSecureRandom): SecureRandom[F] =
diogocanut marked this conversation as resolved.
Show resolved Hide resolved
new ScalaRandom[F](Applicative[F].pure(random), Sync.Type.Blocking) with SecureRandom[F] {}

/**
* Creates a SecureRandom instance. On most platforms, it will be non-blocking. If a
* non-blocking instance can't be guaranteed, falls back to a blocking implementation.
*
* On the JVM, delegates to [[java.security.SecureRandom]].
*
* In browsers, delegates to the
* [[https://developer.mozilla.org/en-US/docs/Web/API/Web_Crypto_API Web Crypto API]].
*
* In Node.js, delegates to the [[https://nodejs.org/api/crypto.html crypto module]].
*
* On Native, delegates to
* [[https://man7.org/linux/man-pages/man3/getentropy.3.html getentropy]] which is supported
* on Linux, macOS, and BSD. Unsupported platforms such as Windows will encounter link-time
* errors.
*/
diogocanut marked this conversation as resolved.
Show resolved Hide resolved
def javaSecuritySecureRandom[F[_]: Sync]: F[SecureRandom[F]] =
Sync[F].delay(unsafeJavaSecuritySecureRandom)

private[effect] def unsafeJavaSecuritySecureRandom[F[_]: Sync]: SecureRandom[F] = {
// This is a known, non-blocking, threadsafe algorithm
def happyRandom = getInstance

def fallback = new JavaSecureRandom()

// Porting JavaSecureRandom.isThreadSafe
def isThreadsafe(rnd: JavaSecureRandom) =
rnd
.getProvider
.getProperty("SecureRandom." + rnd.getAlgorithm + " ThreadSafe", "false")
.toBoolean

// If we can't sniff out a more optimal solution, we can always
// fall back to a pool of blocking instances
def fallbackPool: SecureRandom[F] = {
val processors = Runtime.getRuntime.availableProcessors()
pool(processors)
}

javaMajorVersion match {
case Some(major) if major > 8 =>
try {
// We are thread safe and non-blocking. This is the
// happy path, and happily, the common path.
javaUtilRandom(happyRandom)
} catch {
case _: Throwable =>
diogocanut marked this conversation as resolved.
Show resolved Hide resolved
fallback match {
case rnd if isThreadsafe(rnd) =>
diogocanut marked this conversation as resolved.
Show resolved Hide resolved
// We avoided the mutex, but not the blocking. Use a
// shared instance from the blocking pool.
javaUtilRandomBlocking(rnd)
case _ =>
// We can't prove the instance is threadsafe, so we need
// to pessimistically fall back to a pool. This should
// be exceedingly uncommon.
fallbackPool
}
}

case Some(_) | None =>
// We can't guarantee we're not stuck in a mutex.
fallbackPool
}
}

private def pool[F[_]: Sync](n: Int): SecureRandom[F] = {
val index = new AtomicInteger(0)
val array = Array.fill(n)(new SRandom(new SecureRandom.JavaSecureRandom))

def selectRandom: F[SRandom] = Sync[F].delay {
val currentIndex = index.getAndUpdate(i => if (i < (n - 1)) i + 1 else 0)
diogocanut marked this conversation as resolved.
Show resolved Hide resolved
array(currentIndex)
}

new ScalaRandom[F](selectRandom) with SecureRandom[F] {}
}

private def javaMajorVersion: Option[Int] =
Option(System.getProperty("java.version")).flatMap(parseJavaMajorVersion)

private def parseJavaMajorVersion(javaVersion: String): Option[Int] =
if (javaVersion.startsWith("1."))
Try(javaVersion.split("\\.")(1).toInt).toOption
else
Try(javaVersion.split("\\.")(0).toInt).toOption

}
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,17 @@ import cats.effect.kernel.Sync

import java.util.UUID

private[std] trait UUIDGenCompanionPlatform {
private[std] trait UUIDGenCompanionPlatform extends UUIDGenCompanionPlatformLowPriority

private[std] trait UUIDGenCompanionPlatformLowPriority {

@deprecated(
"Put an implicit `SecureRandom.javaSecuritySecureRandom` into scope to get a more efficient `UUIDGen`, or directly call `UUIDGen.fromSecureRandom`",
"3.6.0"
)
implicit def fromSync[F[_]](implicit ev: Sync[F]): UUIDGen[F] = new UUIDGen[F] {
diogocanut marked this conversation as resolved.
Show resolved Hide resolved
diogocanut marked this conversation as resolved.
Show resolved Hide resolved
djspiewak marked this conversation as resolved.
Show resolved Hide resolved
override final val randomUUID: F[UUID] =
ev.blocking(UUID.randomUUID())
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@

package cats.effect.std

import cats.Applicative
import cats.effect.kernel.Sync
import cats.effect.std.Random.ScalaRandom

import org.typelevel.scalaccompat.annotation._

import scala.scalanative.libc.errno
Expand Down Expand Up @@ -59,6 +63,12 @@ private[std] trait SecureRandomCompanionPlatform {

}

def javaSecuritySecureRandom[F[_]: Sync]: F[SecureRandom[F]] =
Sync[F].delay(unsafeJavaSecuritySecureRandom)

private[effect] def unsafeJavaSecuritySecureRandom[F[_]: Sync]: SecureRandom[F] =
new ScalaRandom[F](Applicative[F].pure(new JavaSecureRandom())) with SecureRandom[F] {}

}

@extern
Expand Down
30 changes: 17 additions & 13 deletions std/shared/src/main/scala/cats/effect/std/Random.scala
Original file line number Diff line number Diff line change
Expand Up @@ -434,17 +434,21 @@ object Random extends RandomCompanionPlatform {

}

private[std] abstract class ScalaRandom[F[_]: Sync](f: F[SRandom]) extends RandomCommon[F] {
private[std] abstract class ScalaRandom[F[_]: Sync](f: F[SRandom], hint: Sync.Type)
extends RandomCommon[F] {

def this(f: F[SRandom]) = this(f, Sync.Type.Delay)

def nextBoolean: F[Boolean] =
for {
r <- f
out <- Sync[F].delay(r.nextBoolean())
out <- Sync[F].suspend(hint)(r.nextBoolean())
} yield out

def nextBytes(n: Int): F[Array[Byte]] =
for {
r <- f
out <- Sync[F].delay {
out <- Sync[F].suspend(hint) {
val bytes = new Array[Byte](0 max n)
r.nextBytes(bytes)
bytes
Expand All @@ -454,61 +458,61 @@ object Random extends RandomCompanionPlatform {
def nextDouble: F[Double] =
for {
r <- f
out <- Sync[F].delay(r.nextDouble())
out <- Sync[F].suspend(hint)(r.nextDouble())
} yield out

def nextFloat: F[Float] =
for {
r <- f
out <- Sync[F].delay(r.nextFloat())
out <- Sync[F].suspend(hint)(r.nextFloat())
} yield out

def nextGaussian: F[Double] =
for {
r <- f
out <- Sync[F].delay(r.nextGaussian())
out <- Sync[F].suspend(hint)(r.nextGaussian())
} yield out

def nextInt: F[Int] =
for {
r <- f
out <- Sync[F].delay(r.nextInt())
out <- Sync[F].suspend(hint)(r.nextInt())
} yield out

def nextIntBounded(n: Int): F[Int] =
for {
r <- f
out <- Sync[F].delay(r.self.nextInt(n))
out <- Sync[F].suspend(hint)(r.self.nextInt(n))
} yield out

def nextLong: F[Long] =
for {
r <- f
out <- Sync[F].delay(r.nextLong())
out <- Sync[F].suspend(hint)(r.nextLong())
} yield out

def nextPrintableChar: F[Char] =
for {
r <- f
out <- Sync[F].delay(r.nextPrintableChar())
out <- Sync[F].suspend(hint)(r.nextPrintableChar())
} yield out

def nextString(length: Int): F[String] =
for {
r <- f
out <- Sync[F].delay(r.nextString(length))
out <- Sync[F].suspend(hint)(r.nextString(length))
} yield out

def shuffleList[A](l: List[A]): F[List[A]] =
for {
r <- f
out <- Sync[F].delay(r.shuffle(l))
out <- Sync[F].suspend(hint)(r.shuffle(l))
} yield out

def shuffleVector[A](v: Vector[A]): F[Vector[A]] =
for {
r <- f
out <- Sync[F].delay(r.shuffle(v))
out <- Sync[F].suspend(hint)(r.shuffle(v))
} yield out
}

Expand Down
7 changes: 3 additions & 4 deletions std/shared/src/main/scala/cats/effect/std/SecureRandom.scala
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,7 @@ object SecureRandom extends SecureRandomCompanionPlatform {
* on Linux, macOS, and BSD. Unsupported platforms such as Windows will encounter link-time
* errors.
*/
def javaSecuritySecureRandom[F[_]: Sync]: F[SecureRandom[F]] =
Sync[F]
.delay(new JavaSecureRandom())
.map(r => new ScalaRandom[F](Applicative[F].pure(r)) with SecureRandom[F] {})
override def javaSecuritySecureRandom[F[_]: Sync]: F[SecureRandom[F]] =
diogocanut marked this conversation as resolved.
Show resolved Hide resolved
super.javaSecuritySecureRandom[F]
Copy link
Contributor

Choose a reason for hiding this comment

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

The other overload above (line 114) refers to this method. However, the behavior of the 2 are now quite different. I think the other overload could also benefit from the new improvements, although the n: Int does not make a lot of sense for the best case (nonblocking) PRNG.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for the best case n: Int. It will only sense in the worst case, when we fall back to a pool.

  private def pool[F[_]: Sync](n: Int): SecureRandom[F] = {
    val index = new AtomicInteger(0)
    val array = Array.fill(n)(new SRandom(new SecureRandom.JavaSecureRandom))

    def selectRandom: F[SRandom] = Sync[F].delay {
      val currentIndex = index.getAndUpdate(i => if (i < (n - 1)) i + 1 else 0)
      array(currentIndex)
    }

    new ScalaRandom[F](selectRandom) with SecureRandom[F] {}
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't very clear: I'm thinking about the fact that the method on line 116 is creating an rng which is "sharded", but calls (potentially) blocking code in delay. The latter is solved by this PR for the other overload. I think it should be solved for the overload in line 116 too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, what's your suggestion for implementing method on line 116 with our new implementation of javaSecuritySecureRandom?


}
20 changes: 20 additions & 0 deletions std/shared/src/main/scala/cats/effect/std/UUIDGen.scala
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,24 @@ object UUIDGen extends UUIDGenCompanionPlatform {

def randomUUID[F[_]: UUIDGen]: F[UUID] = UUIDGen[F].randomUUID
def randomString[F[_]: UUIDGen: Functor]: F[String] = randomUUID.map(_.toString)

implicit def fromSecureRandom[F[_]: Functor: SecureRandom]: UUIDGen[F] =
new UUIDGen[F] {
override final val randomUUID: F[UUID] =
SecureRandom[F].nextBytes(16).map(unsafeUUIDBuilder)

private def unsafeUUIDBuilder(buffer: Array[Byte]): UUID = {
@inline def intFromBuffer(i: Int): Int =
(buffer(i) << 24) | ((buffer(i + 1) & 0xff) << 16) | ((buffer(
i + 2) & 0xff) << 8) | (buffer(i + 3) & 0xff)

val i1 = intFromBuffer(0)
val i2 = (intFromBuffer(4) & ~0x0000f000) | 0x00004000
val i3 = (intFromBuffer(8) & ~0xc0000000) | 0x80000000
val i4 = intFromBuffer(12)
val msb = (i1.toLong << 32) | (i2.toLong & 0xffffffffL)
val lsb = (i3.toLong << 32) | (i4.toLong & 0xffffffffL)
new UUID(msb, lsb)
}
}
}