alexandru commented on code in PR #371: URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1226264411
########## 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: These volatile variables are weird and I don't understand their purpose: ``` @volatile private var index = 0L @volatile private var increment = 0L @volatile private var count = 0L @volatile private var limit = 0 ``` First, `nextId` is definitely not thread-safe as is. Volatile variables are useful to ensure update ordering. So, for example, `index` seems to be updated before `limit`, and the volatile annotation ensures that once the update to `limit` *is seen*, then the update to `index` *is also seen*. This is a very subtle guarantee by Java's memory model that can greatly increase performance (for lock-free concurrent algorithms), but has to be used with care. Adding `@volatile` to variables without thinking about ordering (documented, ideally) does not improve the code, quite the contrary, as it affects performance. If this code is single-threaded, then there is no need for the `@volatile` annotations. Maybe there's a relevant ordering or guarantees that I'm missing, but if so, it needs to be documented. And if this code gets called concurrently, then it would be better to make them thread-safe proper. 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. -- 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]
