mdedetrich commented on code in PR #371: URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1226321926
########## actor/src/main/scala/org/apache/pekko/util/UniqueRandomShortProvider.scala: ########## @@ -0,0 +1,106 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * license agreements; and to You under the Apache License, version 2.0: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * This file is part of the Apache Pekko project, derived from Akka. + */ + +package org.apache.pekko.util + +import org.apache.pekko.annotation.InternalApi + +import java.security.SecureRandom + +/** + * An efficient collision free threadsafe generator of pseudo random + * shorts based on https://en.wikipedia.org/wiki/Double_hashing#Enhanced_double_hashing + * using SecureRandom as a seed. + * + * The avoidance of collisions is achieved by using a Short Array buffer + * that is prefilled with a dimension of 65536 (~ 131072 bytes in size per + * instance of [[UniqueRandomShortProvider]]). This means it will loop + * through all possible 65536 values before starting to re-generating + * the same numbers again. + */ + +@InternalApi +private[pekko] class UniqueRandomShortProvider { + import UniqueRandomShortProvider._ + + @volatile private var index = 0L + @volatile private var increment = 0L + @volatile private var count = 0L + @volatile private var limit = 0 + private val buffer: Array[Short] = new Array[Short](UniqueRandomShortProvider.MaxLimit) + val r = new SecureRandom() + + resetBuffer() + resetSeed() + + count = 1 + + private def resetSeed(): Unit = { + index = r.nextLong + increment = r.nextLong + } + + private def resetBuffer(): Unit = { + System.arraycopy(Filler, 0, buffer, 0, UniqueRandomShortProvider.MaxLimit) Review Comment: > The original code doesn't seem to worry about concurrency, and given that this PR just wants to introduce randomness, it's probably better to just drop those annotations — as they also introduced confusion, e.g., is this code supposed to be thread-safe or not; future readers will also wonder. Definitely agreed, @Claudenw (who wrote the algoritm) did state that its not thread safe so we should just drop that `volatile`'s that he added there otherwise its giving false impressions. However lets wait for https://github.com/apache/incubator-pekko/pull/371#issuecomment-1585680543 @alexandru Regarding scalafmt, its run on every CI run via the usage of scalafmt-native, see https://github.com/apache/incubator-pekko/actions/runs/5220862460/jobs/9424448652?pr=371 and https://github.com/apache/incubator-pekko/blob/main/.github/workflows/format.yml. Its actually not possible to merge a PR if it hasn't been formatted with scalafmt. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
