You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "mdedetrich (via GitHub)" <gi...@apache.org> on 2023/06/07 09:17:42 UTC
[GitHub] [incubator-pekko] mdedetrich opened a new pull request, #371: Generate random DNS id's
mdedetrich opened a new pull request, #371:
URL: https://github.com/apache/incubator-pekko/pull/371
DNS id generated should be random, see https://datatracker.ietf.org/doc/html/rfc5452#section-4.3. One additional thing that needs to be confirmed is whether we need to handle collisions
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on pull request #371: Generate random DNS id's
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1581834847
@pjfanning I think that can be tested, but I will not have much time before weekend.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1593607957
So I have just pushed the branch using the simple version of enhanced double hash with the `synchronized` keyword. Here are the benchmarks
```
[info] IdGeneratorBanchmark.measureEnhancedDoubleHash thrpt 3 0,377 ± 0,032 ops/ns
[info] IdGeneratorBanchmark.measureSecureRandom thrpt 3 0,001 ± 0,001 ops/ns
[info] IdGeneratorBanchmark.measureThreadLocalRandom thrpt 3 0,308 ± 0,057 ops/ns
[info] IdGeneratorBanchmark.multipleThreadsMeasureEnhancedDoubleHash thrpt 3 0,025 ± 0,013 ops/ns
[info] IdGeneratorBanchmark.multipleThreadsMeasureSecureRandom thrpt 3 0,001 ± 0,001 ops/ns
[info] IdGeneratorBanchmark.multipleThreadsMeasureThreadLocalRandom thrpt 3 0,609 ± 0,014 ops/ns
```
Note that I have modified the benchmarks to be more indicative of what we are actually testing in reality, i.e. the original benchmarks designated 8 threads however its unlikely in reality that we will have more than 8 threads hitting the `nextId()` function at once with such high contention, so instead I set the default to just a single thread but added another set of benchmarks for multiple threads.
@pjfanning @IainHull Let me know if I may have missed something
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1230234594
##########
actor/src/main/scala/org/apache/pekko/util/UniqueRandomShortProvider.scala:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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._
+
+ // These methods are volatile so that they stay within cache
+ @volatile private var index = 0L
Review Comment:
So I just spoke with him and he said that he did put volatile there deliberately, its a trick to keep the cache warm (see https://github.com/apache/incubator-pekko/pull/371#discussion_r1228105897).
I will do another set of benchmarks without volatile to see if it makes a difference
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1592071494
So I just rebased the PR with @Claudenw 's latest algorithm and one thing we can say, according to benchmarks its pretty damn fast
```
[info] Benchmark Mode Cnt Score Error Units
[info] IdGeneratorBanchmark.measureEnhancedDoubleHash thrpt 3 ≈ 10⁻⁴ ops/ns
[info] IdGeneratorBanchmark.measureSecureRandom thrpt 3 0,001 ± 0,001 ops/ns
[info] IdGeneratorBanchmark.measureThreadLocalRandom thrpt 3 2,286 ± 0,453 ops/ns
```
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580487442
> Do we know what happens if we change `pekko.io.dns.resolver` to use `inet-address` instead of `async-dns`?
On the top of my head this uses a completely separate implementation from Java stdlib so it shouldn't be an issue.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #371: Generate random DNS id's
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1221557558
##########
actor/src/main/scala/org/apache/pekko/io/dns/internal/AsyncDnsResolver.scala:
##########
@@ -85,11 +86,25 @@ private[io] final class AsyncDnsResolver(
settings.SearchDomains,
settings.NDots)
- private var requestId: Short = 0
+ private var (idIndex, idIncrement) = {
+ val secureRandom = new SecureRandom()
+ (secureRandom.nextLong(), secureRandom.nextLong())
+ }
+
+ private var idCount = 1
+ // DNS id's need to be random, see https://datatracker.ietf.org/doc/html/rfc5452#section-4.3.
+ // Uses enhanced double hashing technique from
+ // https://en.wikipedia.org/wiki/Double_hashing#Enhanced_double_hashing
private def nextId(): Short = {
- requestId = (requestId + 1).toShort
- requestId
+ val result = (0xFFFFFFFF & idIndex).asInstanceOf[Short]
+ idIndex -= idIncrement
+ // Incorporate the counter into the increment to create a
+ // tetrahedral number additional term.
+ idIncrement -= {
+ idCount += 1; idCount - 1
+ }
+ result
Review Comment:
I was using https://github.com/apache/incubator-pekko/pull/288
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1226293690
##########
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)
+ limit = UniqueRandomShortProvider.MaxLimit
+ }
+ def nextId(): Short = {
Review Comment:
There is CI checking - forcing a newline before a new function is just not one of the rules that our scalafmt enforces.
I agree about keeping scalafmt conversations out of PRs when there is automated checking of the agreed formatting already. Separate conversations can be had about changing the format rules.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1584238640
This is just my view but if we can get a solution working that cycles through a large set of possible ids without a collision (like the latest one based on UniqueRandomShortProvider) that that should be enough for Pekko 1.0.0. A solution where we keep track of all in-flight ids might be nice but it would appear to be more expensive than the UniqueRandomShortProvider solution.
In the end of the day, Akka went years without needing a fully collision proof implementation.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on pull request #371: Generate random DNS id's
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1581254502
> > @mdedetrich I don't really mind how this is implemented. If exposing the map state outside of the actor makes this easier to implement, then I suggest that we do that.
> >
> > We can always change that afterwards.
>
> I don't think that would fly in general, I mean it breaks one of the core principles of actors and it would open a whole can of worms
https://github.com/akka/akka/pull/31912
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on pull request #371: Generate random DNS id's
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1582142133
@mdedetrich Yes, that why I had https://github.com/apache/incubator-pekko/pull/288 and https://github.com/akka/akka/pull/31912 for this. This PR can move faster with a smaller scope I think.
$work time, sorry not makes me clear.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1224306820
##########
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:
@kerr we're working on an improvement to this solution and the thread safety issue will probably be easier to fix with this improvement. Let's get the improvement done and we can review if we can also fix that thread safety issue.
The adjustment is to @Claudenw's ShortSupplier and involves a change to stop us from potentially returning recently used IDs when we reach the resetBuffer stage.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1226462262
##########
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:
@IainHull Does it make sense to just make a new PR? Otherwise it seems to be a bit chaotic
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1592940763
@alexandru Thanks for the reply
So to provide some context on how, why and where the concurrency in this specific circumstance matters the code we are dealing with is entirely inside an `Actor` `receive` block which is actually normally meant to be guaranteed to be thread safe (i.e. single thread), the reason why we need to make `nextId()` thread safe is because there is a `recoverWith` on a `Future` which ends up transitively calling `nextId()` within an `ExecutionContext`. This `ExecutionContext` is configurable and hence parameterised, its using the Akka default dispatcher which iirc is a `ForkJoinPool`.
I have strong suspicion that we should be optimising for our happy case, because as you can infer from the usage of `recoverWith` this is probably not going to happen that often so while the usage of `synchronized` is typically ill advised, as you pointed out it is heavily optimized/uses assumptions that mirror the case we are dealing with.
The thing is (and this is a general statement and not strictly about this concurrency issue at hand), its hard for us to get some real data on this and there are many variables at play. For example there are actually 3 versions of @Claudenw algorithm, each with different guarantees of how we want to handle collisions. The "simple" version doesn't handle collisions at all which with initial impressions appears might be the best solution because its going to get handled by the duplicate id changes merged at https://github.com/apache/incubator-pekko/pull/385. However if we get lots of collisions/transactions then one of his variants which mitigate collisions might end up overall being faster.
And while we have a concurrency issue at hand, at the same time we are dealing with random numbers here which means we don't care about "wrong" answers, only if we get duplicates that we can't detect which might open up for some interesting optimisations.
Nonetheless we are trying to work out these concurrency issues and very much appreciate your experience here (if you have some extra time to spare let me know and we can co-ordinate).
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1231912176
##########
actor/src/main/scala/org/apache/pekko/util/RandomShortProvider.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.util.Random
+
+/**
+ * An efficient thread safe generator of pseudo random shorts based on
+ * https://en.wikipedia.org/wiki/Double_hashing#Enhanced_double_hashing.
+ *
+ * Note that due to the usage of synchronized this method is optimized
+ * for the happy case (i.e. least contention) on multiple threads.
+ */
+
+@InternalApi
+private[pekko] class RandomShortProvider(seed: Random) {
+ // These methods are volatile so that they stay within cache
+ private var index = seed.nextLong
+ private var increment = seed.nextLong
+ private var count = 1L
+
+ def nextId(): Short = synchronized {
+ val result = (0xFFFFFFFF & index).asInstanceOf[Short]
Review Comment:
So using a much shorter timeout the improvements are better (i.e. this is the results with 10 nanos)
```
[info] IdGeneratorBanchmark.measureEnhancedDoubleHash thrpt 3 0,001 ± 0,001 ops/ns
[info] IdGeneratorBanchmark.measureSecureRandom thrpt 3 0,001 ± 0,001 ops/ns
[info] IdGeneratorBanchmark.measureThreadLocalRandom thrpt 3 0,312 ± 0,074 ops/ns
[info] IdGeneratorBanchmark.multipleThreadsMeasureEnhancedDoubleHash thrpt 3 0,001 ± 0,001 ops/ns
[info] IdGeneratorBanchmark.multipleThreadsMeasureSecureRandom thrpt 3 0,001 ± 0,001 ops/ns
[info] IdGeneratorBanchmark.multipleThreadsMeasureThreadLocalRandom thrpt 3 0,622 ± 0,018 ops/ns
```
I am not sure what problem we are trying to achieve with a semaphore though, in such a hot loop with only a single permit its actually slower than `synchronized` which because the JVM know its only single threaded (i.e. permit of 1) is hyper optimized for this case.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580449634
> The collision can happen in DnsClient.scala, if there are two flight queries with same id.
>
> And i was.submited a pr in akka for this, but can be done with a fsm like thing too.
If this is regarding it not being thread safe we can solve this separately. Let's fix one problem at a time, the RC is on our doorstep.
On another note CI is failing because the tests need to be updated. I'll do this in a bit.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on pull request #371: Generate random DNS id's
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580396494
The collision can happen in DnsClient.scala, if there are two flight queries with same id.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580973268
Regarding the hash collision problem, this is either going to be non trivial to implement or use a lot of memory and not be correct. A trivial/simple way to implement this is to just have a growing bounded queue which maintains all of the ids that have been generated and to check against if a collision has occurred, dropping the latest elements of the queue if it hits the bound. The problem here is that its not a proper solution (its still theoretically possible to create a collision) and it uses a lot of memory.
The proper solution which other DNS resolvers do (such as systemd-resolverd) is that whenever a new id is created its checked against the current in flight ids and if there is a duplicate then we re-create the id. The reason why we track against current in transaction ids is that hash collisions are fine if there is no context of the id of the colliding hashes, its only a problem if there are currently existing transactions that contain the newly created colliding id.
Thing is, the proper solution is quite complicated and this is due to having to share state, and since we are dealing with actors you are not meant to share any mutable state, instead you are meant to be sending messages (thats the whole point of actors). The state in question that we need to access is
https://github.com/apache/incubator-pekko/blob/4ac0f00a477873965ee7d52e16faefb1de91fe3a/actor/src/main/scala/org/apache/pekko/io/dns/internal/DnsClient.scala#L58 (specifically we only care about the keys of the `Map`).
Initially I thought it would have been possible to just have a `currentIds` collection inside of `AsyncDnsResolver`, adding id's is easy since `nextId()` is also defined inside of `AsyncDnsResolver`, the problematic part is removing the id's. If you check occurrences of `inflightRequests -= <ID>` you can see there are 3 cases where id's are removed (i.e. dropped requests, failed UDP packets etc etc) and only in one of those cases (specifically `DropRequest`) does `AsyncDnsResolver` have context of this.
In conclusion, the proper solution for this would be to add message types to `DnsQuestion`, i.e. `AskIpsInFlight` and `AnswerIpsInFlight`, implement them and then do a scatter + gather implementation in `AsyncDnsResolver` to see if a specific id is inside all of the [resolvers](https://github.com/apache/incubator-pekko/blob/8c346e215e499ed5abcdcf5f2857c3fa3b979dc4/actor/src/main/scala/org/apache/pekko/io/dns/internal/AsyncDnsResolver.scala#L110). Since this is performance sensitive code we shouldn't be using `?`, which further complicates the problem.
I mean a very hacky way to solve this would be to access the state in `DnsClient` actor directly, but that would open up a huge can of works. What does everyone think?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1224221048
##########
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:
I think we'll need to synchronize this block.
```
resetBuffer()
resetSeed()
```
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #371: Generate random DNS id's
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1224214039
##########
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)
+ limit = UniqueRandomShortProvider.MaxLimit
+ }
+ def nextId(): Short = {
Review Comment:
Add a new line.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1231976339
##########
actor/src/main/scala/org/apache/pekko/io/dns/IdGenerator.scala:
##########
@@ -58,4 +56,37 @@ private[pekko] object IdGenerator {
*/
def random(rand: java.util.Random): IdGenerator =
() => (rand.nextInt(MaxUnsignedShort) - Short.MinValue).toShort
+
+ private[pekko] class EnhancedDoubleHashGenerator(semaphoreTimeout: FiniteDuration, seed: Random) extends IdGenerator {
+
+ /**
+ * An efficient thread safe generator of pseudo random shorts based on
+ * https://en.wikipedia.org/wiki/Double_hashing#Enhanced_double_hashing.
+ *
+ * Note that due to the usage of a semaphore this method is optimized
+ * for the happy case (i.e. least contention) on multiple threads.
+ */
+ private val lock = new Semaphore(1)
+ private var index = seed.nextLong
+ private var increment = seed.nextLong
+ private var count = 1L
+
+ private val onAcquireFailure = IdGenerator.random(seed)
+
+ override final def nextId(): Short = {
+ if (lock.tryAcquire(semaphoreTimeout.length, semaphoreTimeout.unit)) {
Review Comment:
revert to synchronized if you find it is working better for you
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1231456894
##########
actor/src/main/scala/org/apache/pekko/util/RandomShortProvider.scala:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.util.Random
+
+/**
+ * An efficient thread safe generator of pseudo random shorts based on
+ * https://en.wikipedia.org/wiki/Double_hashing#Enhanced_double_hashing.
+ *
+ * Note that due to the usage of synchronized this method is optimized
+ * for the happy case (i.e. least contention) on multiple threads.
+ */
+
+@InternalApi
+private[pekko] class RandomShortProvider(seed: Random) {
+ // These methods are volatile so that they stay within cache
+ private var index = seed.nextLong
+ private var increment = seed.nextLong
+ private var count = 1L
+
+ count = 1
Review Comment:
line 31 seems unnecessary
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1594370718
> @mdedetrich this PR is showing a Merge Conflict in GitHub
Fixed
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] IainHull commented on pull request #371: Generate random DNS id's
Posted by "IainHull (via GitHub)" <gi...@apache.org>.
IainHull commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1598847616
@mdedetrich Sorry for the delay getting back, this looks good.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] IainHull commented on a diff in pull request #371: Generate random DNS id's
Posted by "IainHull (via GitHub)" <gi...@apache.org>.
IainHull commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1224490114
##########
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:
Adding collision detection to `DnsClient` seems easier than preventing duplicates when you wrap. Then you can use ` java.util.concurrent.ThreadLocalRandom` to generate the random numbers (like netty does).
https://github.com/netty/netty/pull/4455/files
To protect DNS poisoning you should also add a check to the response handler in `DnsClient` to ensure the question in the response matches the question asked. Otherwise an attacker can send spurious DNS answers as UDP is connectionless, and as long as they get the transaction id correct `DnsClient` will assume its the ip address for the question it asked.
I have a patch to akka 2.6 (based solely on the description of the problem in the CVE and the github issue, I have not looked at the code), but I am waiting for clearance to publish it (hopefully monday or tuesday).
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1231977117
##########
actor/src/main/scala/org/apache/pekko/io/dns/IdGenerator.scala:
##########
@@ -58,4 +56,37 @@ private[pekko] object IdGenerator {
*/
def random(rand: java.util.Random): IdGenerator =
() => (rand.nextInt(MaxUnsignedShort) - Short.MinValue).toShort
+
+ private[pekko] class EnhancedDoubleHashGenerator(semaphoreTimeout: FiniteDuration, seed: Random) extends IdGenerator {
+
+ /**
+ * An efficient thread safe generator of pseudo random shorts based on
+ * https://en.wikipedia.org/wiki/Double_hashing#Enhanced_double_hashing.
+ *
+ * Note that due to the usage of a semaphore this method is optimized
+ * for the happy case (i.e. least contention) on multiple threads.
+ */
+ private val lock = new Semaphore(1)
+ private var index = seed.nextLong
+ private var increment = seed.nextLong
+ private var count = 1L
+
+ private val onAcquireFailure = IdGenerator.random(seed)
+
+ override final def nextId(): Short = {
+ if (lock.tryAcquire(semaphoreTimeout.length, semaphoreTimeout.unit)) {
Review Comment:
Okay I will do so
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1592088864
> If we can get the performance into the same ballpark as 'secure-random' then this generator is a useful addition. I would default to 'secure-random' though (unless the EnhancedDoubleHash was made to be faster than secure-random).
So I don't know whats going on here, but I am having a suspicion that the reason for its slowness is all of the collision detection that was added ontop. When I was discussing with @Claudenw I was contemplating reverting the algorithm to the original simple version that had no buffers because even if there are collisions we can now handle them
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1592087126
If we can get the performance into the same ballpark as 'secure-random' then this generator is a useful addition. I would default to 'secure-random' though (unless the EnhancedDoubleHash was made to be faster than secure-random).
One useful aspect of EnhancedDoubleHash is that it would depend much less on the entropy settings of the device that the code was running on. SecureRandom can be very fast on some hardware but can be very slow in low entropy environments. Low entropy environments can be improved using tools like haveged.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1593031773
> > I tried a solution with an AtomicReference around the state but its performance was slower than the synchronization and Semaphore locking variants. Approximately in SecureRandom territory in the 8 threads microbenchmark.
>
> This isn't too surprising, I suspect that if you wanted to get any tangible benefits of `AtomicLong`/`volatile` you would need to use it on the individual variables (i.e. index/counter etc etc) where its needed which is hard to do correctly
Unfortunately, the 3 numbers in the state are all important. And they should be updated together, so I find it hard to see any solution based on AtomicLongs or AtomicReferences.
Using the Semaphore locking has the best result in my testing. Could we just go with that for RC1? We are very likely to need an RC2 so there is time for someone to find a better solution.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1593021023
> I tried a solution with an AtomicReference around the state but its performance was slower than the synchronization and Semaphore locking variants. Approximately in SecureRandom territory in the 8 threads microbenchmark.
This isn't too surprising, I suspect that if you wanted to get any tangible benefits of `AtomicLong`/`volatile` you would need to use it on the individual variables (i.e. index/counter etc etc) where its needed which is hard to do correctly
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580311125
@Claudenw Pinging
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580579424
> > Do we know what happens if we change `pekko.io.dns.resolver` to use `inet-address` instead of `async-dns`?
>
> Java lib based one may be slower and blocking？
From what @raboof said, the reason why the `async-dns` was implemented in the first place is that there are caching issues with the java implementation.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580762816
So I just checked systemd-resolved and it does appear to handle hash collisions, it does this by tracking the state of current transactions (see https://github.com/systemd/systemd/blob/main/src/resolve/resolved-dns-transaction.c#L195-L196). Will work on this now.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1581120664
@mdedetrich I don't really mind how this is implemented. If exposing the map state outside of the actor makes this easier to implement, then I suggest that we do that.
We can always change that afterwards.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580472205
Do we know what changing `pekko.io.dns.resolver` to use `inet-address` instead of `async-dns`?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1592101904
So here is a benchmark of the version that has no buffer to keep track of collisions or rollover detection
```
[info] Benchmark Mode Cnt Score Error Units
[info] IdGeneratorBanchmark.measureEnhancedDoubleHash thrpt 3 1,105 ± 1,203 ops/ns
[info] IdGeneratorBanchmark.measureSecureRandom thrpt 3 0,001 ± 0,001 ops/ns
[info] IdGeneratorBanchmark.measureThreadLocalRandom thrpt 3 2,345 ± 0,602 ops/ns
```
So its significantly faster than `SecureRandom` and only approx twice as slow thread local random
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1230267093
##########
actor/src/main/resources/reference.conf:
##########
@@ -1145,11 +1145,13 @@ pekko {
# other platforms, will default to 1).
ndots = default
- # The policy used to generate dns transaction ids. Options are thread-local-random or secure-random.
- # Defaults to thread-local-random similar to Netty, secure-random produces FIPS compliant random numbers but
- # could block looking for entropy (these are short integers so are easy to bruit-force, use thread-local-random
- # unless you really require FIPS compliant random numbers).
- id-generator-policy = thread-local-random
+ # The policy used to generate dns transaction ids. Options are `thread-local-random`,
+ # `enhanced-double-hash-random` or `secure-random`. Defaults to `enhanced-double-hash-random` which uses an
+ # enhanced double hashing algorithm optimized for minimizing collisions with a FIPS compliant initial seed.
+ # `thread-local-random` is similar to Netty and secure-random produces FIPS compliant random numbers every time
+ # but could block looking for entropy (these are short integers so are easy to brute-force, use
Review Comment:
secure-random isn't in quotes
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] alexandru commented on a diff in pull request #371: Generate random DNS id's
Posted by "alexandru (via GitHub)" <gi...@apache.org>.
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:
```scala
@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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1586919726
I've created a label to track all the issues and PRs related to async DNS resolution. I think we need to prioritise work on solving the Akka-related CVE and not necessarily fix everything that is wrong.
I've created 3 issues to track 3 problems and created a label to link them and the PRs.
https://github.com/apache/incubator-pekko/labels/async-dns-resolver
This PR relates to #384
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1226448404
##########
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:
@IainHull I think that the policies that you support in #385 are sufficient. You have a mechanism for tracking duplicate IDs in your PR. It's possible that there is stuff in this PR that may be a useful addition to the #385 changes but so far, I think they are rival solutions.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580884416
It looks like a singleton actor instance to me - so `receive` should only be processing one message at a time.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1231975453
##########
actor/src/main/scala/org/apache/pekko/util/RandomShortProvider.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.util.Random
+
+/**
+ * An efficient thread safe generator of pseudo random shorts based on
+ * https://en.wikipedia.org/wiki/Double_hashing#Enhanced_double_hashing.
+ *
+ * Note that due to the usage of synchronized this method is optimized
+ * for the happy case (i.e. least contention) on multiple threads.
+ */
+
+@InternalApi
+private[pekko] class RandomShortProvider(seed: Random) {
+ // These methods are volatile so that they stay within cache
+ private var index = seed.nextLong
+ private var increment = seed.nextLong
+ private var count = 1L
+
+ def nextId(): Short = synchronized {
+ val result = (0xFFFFFFFF & index).asInstanceOf[Short]
Review Comment:
@pjfanning You just approved PR, did you read this?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1592141625
So as @pjfanning pointed out, the current benchmarks are somewhat misleading in that they are run over 8 threads which is the complete worst case scenario for `synchronized` especially when taking into account that in almost every case the calls will from the same thread, only when `recoverWith` in the `Future` block gets hit can we get multiple calls.
If we modify benchmark to just use single thread then here is the non `synchronized` version
```
[info] IdGeneratorBanchmark.measureEnhancedDoubleHash thrpt 3 0,441 ± 0,031 ops/ns
[info] IdGeneratorBanchmark.measureSecureRandom thrpt 3 0,001 ± 0,001 ops/ns
[info] IdGeneratorBanchmark.measureThreadLocalRandom thrpt 3 0,313 ± 0,034 ops/ns
```
and synchronized version
```
[info] IdGeneratorBanchmark.measureEnhancedDoubleHash thrpt 3 0,380 ± 0,008 ops/ns
[info] IdGeneratorBanchmark.measureSecureRandom thrpt 3 0,001 ± 0,001 ops/ns
[info] IdGeneratorBanchmark.measureThreadLocalRandom thrpt 3 0,312 ± 0,053 ops/ns
```
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] alexandru commented on pull request #371: Generate random DNS id's
Posted by "alexandru (via GitHub)" <gi...@apache.org>.
alexandru commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1592917793
Adding some educated opinions on concurrency because why not 🙂 ...
If you're thinking of adding `synchronized`, the problem with intrinsic locks is that the JVM tries to be smart about it. Prior versions of the JVM used an algorithm that was biased for single-threading, which I think they pulled out at some point. The JVM also does “escape analysis” and in some cases eliminates intrinsic locks altogether. And occasionally the JVM isn't smart enough. For example, one limitation of Project Loom is that `synchronized` is pinning virtual threads to platform threads. Not to mention that algorithms using `synchronize` can block concurrent threads from making progress by just having the scheduler freeze the thread holding the lock.
I would abstain from using `synchronize` simply because it's magic™️, having unpredictable behavior that's not good enough. `synchronize` should be used when we don't have better ideas or when performance predictability doesn't matter much.
I don't know about processor caching, but I'd also avoid `volatile` variables for the same reason: the JVM does magic. Also, for ensuring read/write ordering, volatile reads/writes introduce memory barriers which is the opposite of caching. Furthermore, those memory barriers aren't reliably inserted, due to the same escape analysis that the JVM can pull off, and only sometimes; plus the introduction of memory barriers depends on reads/writes being done in pairs, similar to acquiring a lock coupled with a release on that same monitor.
IMO, we shouldn't try to outsmart the JVM in these matters, unless we can reason about the ordering of operations.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1581223381
> @mdedetrich What do you think about my PR？I think it's correct too.
Which one?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580689011
I raised https://github.com/apache/incubator-pekko/issues/373 for the lower priority issue of tracking ids
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1582150500
> @mdedetrich Yes, that why I had #288 and [akka/akka#31912](https://github.com/akka/akka/pull/31912) for this. This PR can move faster with a smaller scope I think.
>
> $work time, sorry not makes me clear.
I still think the issue is the same, i.e. `requestIdGen` in https://github.com/apache/incubator-pekko/pull/288/files#diff-4891e059729a403d92d62966e039a3f9b6aa24fbc7562e1ed9daa280c46328d5R97 would need to access `inFlightTransactions` in `DnsClient` actor, but sure lets wait till the weekend when you have time to look at this properly.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1224224317
##########
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:
it's the recoverWith problem - the nextId() can be called by multiple threads - it seems like we need a synchronize somewhere
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #371: Generate random DNS id's
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1224218476
##########
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:
This seems not threadsafe
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] IainHull commented on a diff in pull request #371: Generate random DNS id's
Posted by "IainHull (via GitHub)" <gi...@apache.org>.
IainHull commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1226444066
##########
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:
I have finally got clearance to share my code. I am happy to help combine this with any of the active PRs in this area.
https://github.com/apache/incubator-pekko/pull/385
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] alexandru commented on a diff in pull request #371: Generate random DNS id's
Posted by "alexandru (via GitHub)" <gi...@apache.org>.
alexandru commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1226284401
##########
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)
+ limit = UniqueRandomShortProvider.MaxLimit
+ }
+ def nextId(): Short = {
Review Comment:
Formatting is important, but PR comments on formatting (in large projects) don't scale.
Scalafmt is enabled on the project. I think it's Scalafmt's job to reformat and complain. Its configuration is never perfect, but it avoids the bike shedding in PRs. Does Scalafmt not add/require these new lines? If not, then maybe a configuration change would be needed.
I see there's also a `verifyCodeStyle` command in `build.sbt`, it would be a good idea to include this in CI to have the build fail if the code style fails.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1231471372
##########
actor/src/main/scala/org/apache/pekko/util/RandomShortProvider.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.util.Random
+
+/**
+ * An efficient thread safe generator of pseudo random shorts based on
+ * https://en.wikipedia.org/wiki/Double_hashing#Enhanced_double_hashing.
+ *
+ * Note that due to the usage of synchronized this method is optimized
+ * for the happy case (i.e. least contention) on multiple threads.
+ */
+
+@InternalApi
+private[pekko] class RandomShortProvider(seed: Random) {
+ // These methods are volatile so that they stay within cache
+ private var index = seed.nextLong
+ private var increment = seed.nextLong
+ private var count = 1L
+
+ def nextId(): Short = synchronized {
+ val result = (0xFFFFFFFF & index).asInstanceOf[Short]
Review Comment:
Sure ill look at it tomorrow, will be busy tonight
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1230261109
##########
actor/src/main/resources/reference.conf:
##########
@@ -1145,11 +1145,13 @@ pekko {
# other platforms, will default to 1).
ndots = default
- # The policy used to generate dns transaction ids. Options are thread-local-random or secure-random.
- # Defaults to thread-local-random similar to Netty, secure-random produces FIPS compliant random numbers but
- # could block looking for entropy (these are short integers so are easy to bruit-force, use thread-local-random
- # unless you really require FIPS compliant random numbers).
- id-generator-policy = thread-local-random
+ # The policy used to generate dns transaction ids. Options are thread-local-random, enhanced-double-hash-random
Review Comment:
Could you put the values in quotes? This makes them stand out a bit more. 'bruit' should 'brute' as in brute force.
Could you change 'initial seed, thread-local-random' to this (with a full stop instead of a comma)?
```
initial seed. 'thread-local-random'
```
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1593015247
I tried a solution with an AtomicReference around the state but its performance was slower than the synchronization and Semaphore locking variants. Approximately in SecureRandom territory in the 8 threads microbenchmark.
```
private val state = {
val (index, increment) = {
val r = new SecureRandom()
(r.nextLong, r.nextLong())
}
new AtomicReference(Array(index, increment, 1))
}
@tailrec
final def nextId(): Short = {
val array = state.get()
var index = array(0)
var increment = array(1)
var count = array(2)
val result = (0xFFFFFFFF & index).asInstanceOf[Short]
index -= increment
// Incorporate the counter into the increment to create a
// tetrahedral number additional term.
increment -= {
count += 1
count - 1
}
if (state.compareAndSet(array, Array(index, increment, count))) {
result
} else {
nextId()
}
}
```
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1593037195
> Unfortunately, the 3 numbers in the state are all important. And they should be updated together, so I find it hard to see any solution based on AtomicLongs or AtomicReferences.
Ordinarily yes, but we aren't looking for complete correctness here but I can't think of any way to abuse this atm
> Using the Semaphore locking has the best result in my testing. Could we just go with that for RC1? We are very likely to need an RC2 so there is time for someone to find a better solution.
Fine with me unless there are some other real objections.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1592134199
So here are the results when using a simple faster version with the `synchronized` block
```
[info] Benchmark Mode Cnt Score Error Units
[info] IdGeneratorBanchmark.measureEnhancedDoubleHash thrpt 3 0,009 ± 0,012 ops/ns
[info] IdGeneratorBanchmark.measureSecureRandom thrpt 3 0,001 ± 0,001 ops/ns
[info] IdGeneratorBanchmark.measureThreadLocalRandom thrpt 3 2,312 ± 0,531 ops/ns
```
While its still faster than secure random its now much slower than thread local random, i.e. performance impact was significant. Rewriting the algorithm with AtomicLong/volatile can likely mitigate this greatly
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580348336
> I'm not an expert but any idea why the value is a `short`. If we used a type like `int` or `long`, the risk of collisions reduces.
It has to be according to the standard, see https://datatracker.ietf.org/doc/html/rfc5452#section-4.3. The generated ID's fill a octet (i.e. 2 byte) range which in Java/Scala is a `Short`.
Actually this new implementation is strictly better than the current one because the current one only used 1 byte range since it just incremented from `0` and `Short.MaxValue + 1` doesn't overflow into `Short.MinValue`.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580342872
I'm not an expert but any idea why the value is a `short`. If we used a type like `int` or `long`, the risk of collisions reduces.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1581293239
@He-Pin https://github.com/akka/akka/pull/31912 looks like a viable approach. Is there any way that a unit test could be developed?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1592930109
@alexandru I suggested to @mdedetrich to use a Semaphore instead. It seems a little faster in the microbenchmark but you can set a timeout on the acquire as well.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1230267893
##########
actor/src/main/resources/reference.conf:
##########
@@ -1145,11 +1145,13 @@ pekko {
# other platforms, will default to 1).
ndots = default
- # The policy used to generate dns transaction ids. Options are thread-local-random or secure-random.
- # Defaults to thread-local-random similar to Netty, secure-random produces FIPS compliant random numbers but
- # could block looking for entropy (these are short integers so are easy to bruit-force, use thread-local-random
- # unless you really require FIPS compliant random numbers).
- id-generator-policy = thread-local-random
+ # The policy used to generate dns transaction ids. Options are `thread-local-random`,
+ # `enhanced-double-hash-random` or `secure-random`. Defaults to `enhanced-double-hash-random` which uses an
+ # enhanced double hashing algorithm optimized for minimizing collisions with a FIPS compliant initial seed.
+ # `thread-local-random` is similar to Netty and secure-random produces FIPS compliant random numbers every time
+ # but could block looking for entropy (these are short integers so are easy to brute-force, use
Review Comment:
Fixed
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] alexandru commented on a diff in pull request #371: Generate random DNS id's
Posted by "alexandru (via GitHub)" <gi...@apache.org>.
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. And if this code gets called concurrently, then it would be better to make them thread-safe proper.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] alexandru commented on a diff in pull request #371: Generate random DNS id's
Posted by "alexandru (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] alexandru commented on a diff in pull request #371: Generate random DNS id's
Posted by "alexandru (via GitHub)" <gi...@apache.org>.
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. 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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1231459300
##########
actor/src/main/scala/org/apache/pekko/util/RandomShortProvider.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.util.Random
+
+/**
+ * An efficient thread safe generator of pseudo random shorts based on
+ * https://en.wikipedia.org/wiki/Double_hashing#Enhanced_double_hashing.
+ *
+ * Note that due to the usage of synchronized this method is optimized
+ * for the happy case (i.e. least contention) on multiple threads.
+ */
+
+@InternalApi
+private[pekko] class RandomShortProvider(seed: Random) {
+ // These methods are volatile so that they stay within cache
+ private var index = seed.nextLong
+ private var increment = seed.nextLong
+ private var count = 1L
+
+ def nextId(): Short = synchronized {
+ val result = (0xFFFFFFFF & index).asInstanceOf[Short]
Review Comment:
would it be possible to consider https://github.com/mdedetrich/incubator-pekko/blob/514ac493c3d530aea16f86d702b7d2ed358229e1/actor/src/main/scala/org/apache/pekko/util/UniqueRandomShortProvider.scala ? @alexandru suggested that 'synchronized' is a bad solution and the semaphore can timeout (while synchronized can just hang).
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1224234758
##########
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 thread safety is a known problem and is being addressed separately, we discussed this earlier. I would recommend making the algorithm thread safe later, lets get an initial solution for now.
Using `synchronized` is usually just a heavy handed cop-out due to how slow it is, there are smarter ways to make it thread safe.
@Claudenw confirmed the current implementation isn't thread safe but this should not block us
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580875470
> same
So having gone through the codebase, `nextId()` is only called in a `receive` block (indirectly) which means that its only going to be called in a single threaded context (this is something that actor guarantees).
Did I miss something?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1582068983
> @He-Pin [akka/akka#31912](https://github.com/akka/akka/pull/31912) looks like a viable approach. Is there any way that a unit test could be developed?
So I had a look at this in more detail and I don't think this would solve the specific problem we are dealing with here. Unless I misunderstood, this is solving the issue where you can have multiple in flight requests with the same id where as I am dealing with the issue where we don't want to generate multiple ids by checking in flight transactions.
While the difference is subtle its somewhat critical here, the solution in https://github.com/akka/akka/pull/31912 involves putting guards around `inflightRequests` within `DnsClient` itself where as the problem we have is that we have an external `AsyncDnsResolver` actor that needs to access the state of `DnsClient.
If `inflightTransactions` was hypothetically inside of `AsyncDnsResolver` we wouldn't even be having this issue and the solution would be done by now and this is what I alluded to when I made this earlier statement
> Initially I thought it would have been possible to just have a currentIds collection inside of AsyncDnsResolver, adding id's is easy since nextId() is also defined inside of AsyncDnsResolver, the problematic part is removing the id's. If you check occurrences of inflightRequests -= <ID> you can see there are 3 cases where id's are removed (i.e. dropped requests, failed UDP packets etc etc) and only in one of those cases (specifically DropRequest) does AsyncDnsResolver have context of this.
A hypothetical solution to this issue would be to reverse the problem, i.e. for the other cases where `inflightRequests -= <ID>` occurs in Actors outside of `AsyncDnsResolver`, those actors could send a message to `AsyncDnsResolver` stating that an id has been dropped but the problem here is this would create a cyclic dependency because the relationship is currently the opposite way around, i.e. `AsyncDnsResolver` keeps track of the other actors (such as `DnsClient`) and not the other way around (`DnsClient` does not have any actor references to `AsyncDnsResolver`)
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1231896345
##########
actor/src/main/scala/org/apache/pekko/util/RandomShortProvider.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.util.Random
+
+/**
+ * An efficient thread safe generator of pseudo random shorts based on
+ * https://en.wikipedia.org/wiki/Double_hashing#Enhanced_double_hashing.
+ *
+ * Note that due to the usage of synchronized this method is optimized
+ * for the happy case (i.e. least contention) on multiple threads.
+ */
+
+@InternalApi
+private[pekko] class RandomShortProvider(seed: Random) {
+ // These methods are volatile so that they stay within cache
+ private var index = seed.nextLong
+ private var increment = seed.nextLong
+ private var count = 1L
+
+ def nextId(): Short = synchronized {
+ val result = (0xFFFFFFFF & index).asInstanceOf[Short]
Review Comment:
So I have just update the version to use a semaphore as well as making the lock timeout configurable plus I did some other improvements to make the config reading/parsing consistent with the rest of Pekko.
I used the exact same version that you suggested (with a minor performance improvement to avoid creating a new class in the failure case of acquiring a lock) and the performance nosedived.
```
[info] IdGeneratorBanchmark.measureEnhancedDoubleHash thrpt 3 ≈ 10⁻¹⁰ ops/ns
[info] IdGeneratorBanchmark.measureSecureRandom thrpt 3 0,001 ± 0,001 ops/ns
[info] IdGeneratorBanchmark.measureThreadLocalRandom thrpt 3 0,311 ± 0,031 ops/ns
[info] IdGeneratorBanchmark.multipleThreadsMeasureEnhancedDoubleHash thrpt 3 ≈ 10⁻¹⁰ ops/ns
[info] IdGeneratorBanchmark.multipleThreadsMeasureSecureRandom thrpt 3 0,001 ± 0,001 ops/ns
[info] IdGeneratorBanchmark.multipleThreadsMeasureThreadLocalRandom thrpt 3 0,608 ± 0,036 ops/ns
```
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1230265565
##########
actor/src/main/resources/reference.conf:
##########
@@ -1145,11 +1145,13 @@ pekko {
# other platforms, will default to 1).
ndots = default
- # The policy used to generate dns transaction ids. Options are thread-local-random or secure-random.
- # Defaults to thread-local-random similar to Netty, secure-random produces FIPS compliant random numbers but
- # could block looking for entropy (these are short integers so are easy to bruit-force, use thread-local-random
- # unless you really require FIPS compliant random numbers).
- id-generator-policy = thread-local-random
+ # The policy used to generate dns transaction ids. Options are thread-local-random, enhanced-double-hash-random
Review Comment:
Done
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on pull request #371: Generate random DNS id's
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580309755
1. collisions should be handled too.
2. the call to `nextId` is not thread safe, does this pr handle this?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1581226332
> @mdedetrich I don't really mind how this is implemented. If exposing the map state outside of the actor makes this easier to implement, then I suggest that we do that.
>
> We can always change that afterwards.
I don't think that would fly in general, I mean it breaks one of the core principles of actors and it would open a whole can of worms
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on pull request #371: Generate random DNS id's
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580522912
> Do we know what happens if we change `pekko.io.dns.resolver` to use `inet-address` instead of `async-dns`?
Java lib based one may be slower and blocking？
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580339269
The old code wasn't thread safe either - why do we need to worry about this code being thread safe?
To make the old code thread safe, we'd need to use an AtomicInteger or something like that.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #371: Generate random DNS id's
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1224293850
##########
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:
So @mdedetrich you want to issue another PR for the thread safty?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1584235925
@He-Pin @pjfanning I have updated a PR for a new implementation provided by @Claudenw . The new implementation guarantees no hash collisions until it loops through all possible 65536 values. This is achieved by using a
prefilled array buffer. While this means the algorithm does take up a decent amount of memory (~131072 bytes) this allocation only actually occurs if you call `UniqueRandomShortProvider.nextId()` which means you don't pay for it if you don't use it and 131072 bytes is all things considered a very tiny amount when it comes to modern machines.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1231459300
##########
actor/src/main/scala/org/apache/pekko/util/RandomShortProvider.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.util.Random
+
+/**
+ * An efficient thread safe generator of pseudo random shorts based on
+ * https://en.wikipedia.org/wiki/Double_hashing#Enhanced_double_hashing.
+ *
+ * Note that due to the usage of synchronized this method is optimized
+ * for the happy case (i.e. least contention) on multiple threads.
+ */
+
+@InternalApi
+private[pekko] class RandomShortProvider(seed: Random) {
+ // These methods are volatile so that they stay within cache
+ private var index = seed.nextLong
+ private var increment = seed.nextLong
+ private var count = 1L
+
+ def nextId(): Short = synchronized {
+ val result = (0xFFFFFFFF & index).asInstanceOf[Short]
Review Comment:
would it be possible to consider https://github.com/mdedetrich/incubator-pekko/pull/1 ? @alexandru suggested that 'synchronized' is a bad solution and the semaphore can timeout (while synchronized can just hang).
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1592076317
> So I just rebased the PR with @Claudenw 's latest algorithm and one thing we can say, according to benchmarks its pretty damn fast
>
> ```
> [info] Benchmark Mode Cnt Score Error Units
> [info] IdGeneratorBanchmark.measureEnhancedDoubleHash thrpt 3 ≈ 10⁻⁴ ops/ns
> [info] IdGeneratorBanchmark.measureSecureRandom thrpt 3 0,001 ± 0,001 ops/ns
> [info] IdGeneratorBanchmark.measureThreadLocalRandom thrpt 3 2,286 ± 0,453 ops/ns
> ```
Isn't 10e-4 a small number meaning poor performance?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1231981449
##########
actor/src/main/scala/org/apache/pekko/io/dns/IdGenerator.scala:
##########
@@ -58,4 +56,37 @@ private[pekko] object IdGenerator {
*/
def random(rand: java.util.Random): IdGenerator =
() => (rand.nextInt(MaxUnsignedShort) - Short.MinValue).toShort
+
+ private[pekko] class EnhancedDoubleHashGenerator(semaphoreTimeout: FiniteDuration, seed: Random) extends IdGenerator {
+
+ /**
+ * An efficient thread safe generator of pseudo random shorts based on
+ * https://en.wikipedia.org/wiki/Double_hashing#Enhanced_double_hashing.
+ *
+ * Note that due to the usage of a semaphore this method is optimized
+ * for the happy case (i.e. least contention) on multiple threads.
+ */
+ private val lock = new Semaphore(1)
+ private var index = seed.nextLong
+ private var increment = seed.nextLong
+ private var count = 1L
+
+ private val onAcquireFailure = IdGenerator.random(seed)
+
+ override final def nextId(): Short = {
+ if (lock.tryAcquire(semaphoreTimeout.length, semaphoreTimeout.unit)) {
Review Comment:
btw I just merged a PR fixing a small issue in the negative ID fix - I have resolved the conflict in your PR/branch - so you might want to pull the latest code
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich merged pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich merged PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1594367182
@mdedetrich this PR is showing a Merge Conflict in GitHub
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
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 (see https://github.com/apache/incubator-pekko/blob/main/.asf.yaml#L43)
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580703967
> I raised #373 for the lower priority issue of tracking ids
I am going to confirm whether collisions are an issue. Note that one advantage of the algorithm that @Claudenw provided is that its deliberately designed to minimize the amount of collisions, from https://en.wikipedia.org/wiki/Double_hashing#Enhanced_double_hashing
> the interval depends on the data, so that values mapping to the same location have different bucket sequences; this minimizes repeated collisions and the effects of clustering.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #371: Generate random DNS id's
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1221553158
##########
actor/src/main/scala/org/apache/pekko/io/dns/internal/AsyncDnsResolver.scala:
##########
@@ -85,11 +86,25 @@ private[io] final class AsyncDnsResolver(
settings.SearchDomains,
settings.NDots)
- private var requestId: Short = 0
+ private var (idIndex, idIncrement) = {
+ val secureRandom = new SecureRandom()
+ (secureRandom.nextLong(), secureRandom.nextLong())
+ }
+
+ private var idCount = 1
+ // DNS id's need to be random, see https://datatracker.ietf.org/doc/html/rfc5452#section-4.3.
+ // Uses enhanced double hashing technique from
+ // https://en.wikipedia.org/wiki/Double_hashing#Enhanced_double_hashing
private def nextId(): Short = {
- requestId = (requestId + 1).toShort
- requestId
+ val result = (0xFFFFFFFF & idIndex).asInstanceOf[Short]
+ idIndex -= idIncrement
+ // Incorporate the counter into the increment to create a
+ // tetrahedral number additional term.
+ idIncrement -= {
+ idCount += 1; idCount - 1
+ }
+ result
Review Comment:
`nextId` can be called concurrently, so I think two thread can get the same result.
`idIncrement` and `idIndex` should be protected with `AtomicLong`
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on pull request #371: Generate random DNS id's
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1589123754
@mdedetrich In recent Netty PR:
refs: https://github.com/netty/netty/pull/13439/
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] Claudenw commented on a diff in pull request #371: Generate random DNS id's
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1228105897
##########
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 volatile keyword was to keep the vars in cache as much as possible, they can be removed. However `UniqueRandomShortProvider.nextId()` needs to be atomic for thread safety.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1226468102
##########
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:
@mdedetrich I think the idea is that #385 could be merged instead of this. Is there anything that #385 is missing that we would need for a Pekko 1.0.0 release?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on pull request #371: Generate random DNS id's
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1581218288
@mdedetrich What do you think about my PR？I think that's correct too.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #371: Generate random DNS id's
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1221553158
##########
actor/src/main/scala/org/apache/pekko/io/dns/internal/AsyncDnsResolver.scala:
##########
@@ -85,11 +86,25 @@ private[io] final class AsyncDnsResolver(
settings.SearchDomains,
settings.NDots)
- private var requestId: Short = 0
+ private var (idIndex, idIncrement) = {
+ val secureRandom = new SecureRandom()
+ (secureRandom.nextLong(), secureRandom.nextLong())
+ }
+
+ private var idCount = 1
+ // DNS id's need to be random, see https://datatracker.ietf.org/doc/html/rfc5452#section-4.3.
+ // Uses enhanced double hashing technique from
+ // https://en.wikipedia.org/wiki/Double_hashing#Enhanced_double_hashing
private def nextId(): Short = {
- requestId = (requestId + 1).toShort
- requestId
+ val result = (0xFFFFFFFF & idIndex).asInstanceOf[Short]
+ idIndex -= idIncrement
+ // Incorporate the counter into the increment to create a
+ // tetrahedral number additional term.
+ idIncrement -= {
+ idCount += 1; idCount - 1
+ }
+ result
Review Comment:
`nextId` can be called concurrently, so I think two thread can get the same result.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on pull request #371: Generate random DNS id's
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580996162
> > `nextId` can be called concurrently, so I think two thread can get the same result. `idIncrement` and `idIndex` should be protected with `AtomicLong`
>
> So having gone through the codebase, `nextId()` is only called in a `receive` block (indirectly) which means that its only going to be called in a single threaded context (this is something that actor guarantees).
>
> Did I miss something?
IIRC, it can be called from the `recover` of the future。
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1231457724
##########
actor/src/main/scala/org/apache/pekko/util/RandomShortProvider.scala:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.util.Random
+
+/**
+ * An efficient thread safe generator of pseudo random shorts based on
+ * https://en.wikipedia.org/wiki/Double_hashing#Enhanced_double_hashing.
+ *
+ * Note that due to the usage of synchronized this method is optimized
+ * for the happy case (i.e. least contention) on multiple threads.
+ */
+
+@InternalApi
+private[pekko] class RandomShortProvider(seed: Random) {
+ // These methods are volatile so that they stay within cache
+ private var index = seed.nextLong
+ private var increment = seed.nextLong
+ private var count = 1L
+
+ count = 1
Review Comment:
Fixed, thanks
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1230267093
##########
actor/src/main/resources/reference.conf:
##########
@@ -1145,11 +1145,13 @@ pekko {
# other platforms, will default to 1).
ndots = default
- # The policy used to generate dns transaction ids. Options are thread-local-random or secure-random.
- # Defaults to thread-local-random similar to Netty, secure-random produces FIPS compliant random numbers but
- # could block looking for entropy (these are short integers so are easy to bruit-force, use thread-local-random
- # unless you really require FIPS compliant random numbers).
- id-generator-policy = thread-local-random
+ # The policy used to generate dns transaction ids. Options are `thread-local-random`,
+ # `enhanced-double-hash-random` or `secure-random`. Defaults to `enhanced-double-hash-random` which uses an
+ # enhanced double hashing algorithm optimized for minimizing collisions with a FIPS compliant initial seed.
+ # `thread-local-random` is similar to Netty and secure-random produces FIPS compliant random numbers every time
+ # but could block looking for entropy (these are short integers so are easy to brute-force, use
Review Comment:
should be `secure-random` to match the others
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1231896345
##########
actor/src/main/scala/org/apache/pekko/util/RandomShortProvider.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.util.Random
+
+/**
+ * An efficient thread safe generator of pseudo random shorts based on
+ * https://en.wikipedia.org/wiki/Double_hashing#Enhanced_double_hashing.
+ *
+ * Note that due to the usage of synchronized this method is optimized
+ * for the happy case (i.e. least contention) on multiple threads.
+ */
+
+@InternalApi
+private[pekko] class RandomShortProvider(seed: Random) {
+ // These methods are volatile so that they stay within cache
+ private var index = seed.nextLong
+ private var increment = seed.nextLong
+ private var count = 1L
+
+ def nextId(): Short = synchronized {
+ val result = (0xFFFFFFFF & index).asInstanceOf[Short]
Review Comment:
So I have just update the version to use a semaphore as well as making the timeout configurable plus I did some other improvements to make the config reading/parsing consistent with the rest of Pekko.
I used the exact same version that you suggested (with a minor performance improvement to avoid creating a new class in the failure case of acquiring a lock) and the performance nosedived.
```
[info] IdGeneratorBanchmark.measureEnhancedDoubleHash thrpt 3 ≈ 10⁻¹⁰ ops/ns
[info] IdGeneratorBanchmark.measureSecureRandom thrpt 3 0,001 ± 0,001 ops/ns
[info] IdGeneratorBanchmark.measureThreadLocalRandom thrpt 3 0,311 ± 0,031 ops/ns
[info] IdGeneratorBanchmark.multipleThreadsMeasureEnhancedDoubleHash thrpt 3 ≈ 10⁻¹⁰ ops/ns
[info] IdGeneratorBanchmark.multipleThreadsMeasureSecureRandom thrpt 3 0,001 ± 0,001 ops/ns
[info] IdGeneratorBanchmark.multipleThreadsMeasureThreadLocalRandom thrpt 3 0,608 ± 0,036 ops/ns
```
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1230238414
##########
actor/src/main/scala/org/apache/pekko/util/UniqueRandomShortProvider.scala:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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._
+
+ // These methods are volatile so that they stay within cache
+ @volatile private var index = 0L
Review Comment:
I removed them as they don't seem to be making a difference
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1585680543
So just to clarify this a bit more, the current solution in this PR is not ideal and both @pjfanning, @Claudenw and myself have an improvement however I am holding off adding this due to what @IainHull posted here https://github.com/apache/incubator-pekko/pull/371#discussion_r1224490114 since it may be a waste of time.
Hopefully the legal hurdles will be overcome and we can discuss and review that solution comprehensively.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1598885347
@IainHull Note that I made a ticket for anyone that wants to further improve performance of double enhanced hasher https://github.com/apache/incubator-pekko/issues/411
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580695434
So I have just updated the tests and rebased/force pushed the changes. The reason why the tests are failing is that they assumed the `nextId` was incrementally increasing and hence checking against raw id's. I have updated the tests so that we don't test against the id's but the properties for the id, i.e. typically speaking the test expects an initial id from the `dnsClient1` i.e.
```scala
val firstId = dnsClient1.expectMsgPF() {
case q4: Question4 if q4.name == "cats.com" =>
q4.id
}
```
and then that `secondId` is being reused in later on, such as
```scala
val secondId = dnsClient2.expectMsgPF() {
case q4: Question4 if q4.name == "cats.com" && q4.id != firstId =>
q4.id
}
```
i.e. in this specific case we can't test for hardcoded id's but we can still make sure that the `secondId` is not the same as the `firstId`.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580332039
> collisions should be handled too.
So this isn't entirely clear because I have seen other implementations of DNS resolvers in other software (i.e. systemd-resolved) and they just use standard random which is also prone to collisions.
> the call to nextId is not thread safe, does this pr handle this?
I don't think this is an issue, the entire implementation is private and I don't think it can be called from multiple threads at once. Even if it can then its not a problem, you might get a slightly "wrong" result but it doesn't matter because we just want random numbers.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1581007514
> IIRC, it can be called from the recover of the future。
Ah yes, I can see it, https://github.com/apache/incubator-pekko/blob/8c346e215e499ed5abcdcf5f2857c3fa3b979dc4/actor/src/main/scala/org/apache/pekko/io/dns/internal/AsyncDnsResolver.scala#L163
As mentioned before I would still treat this as separately because as a result of https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580973268 we may end up changing the internals of this codebase a fair bit.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1224234758
##########
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 thread safety is a known problem and is being addressed separately, we discussed this earlier. I would recommend making the algorithm thread safe later, lets get an initial solution for now.
Using `synchronized` is usually just a heavy handed cop-out due to how slow it is, there are smarter ways to make it thread safe.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #371: Generate random DNS id's
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1224213414
##########
actor/src/main/scala/org/apache/pekko/io/dns/internal/AsyncDnsResolver.scala:
##########
@@ -85,12 +85,15 @@ private[io] final class AsyncDnsResolver(
settings.SearchDomains,
settings.NDots)
- private var requestId: Short = 0
+ private val shortSupplier = new UniqueRandomShortProvider()
+ // Since ShortSupplier has lazy initialization lets immediately
+ // call nextId to warm it up so that next time its actually needed
+ // it will be fast
+ shortSupplier.nextId()
- private def nextId(): Short = {
- requestId = (requestId + 1).toShort
- requestId
- }
+ // DNS id's need to be random, see https://datatracker.ietf.org/doc/html/rfc5452#section-4.3.
+ private def nextId(): Short =
+ shortSupplier.nextId()
Review Comment:
Do not need to start a new line.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #371: Generate random DNS id's
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1224213622
##########
actor/src/main/scala/org/apache/pekko/io/dns/internal/AsyncDnsResolver.scala:
##########
@@ -257,4 +260,5 @@ private[pekko] object AsyncDnsResolver {
Future.successful(Answer(-1, immutable.Seq.empty[ResourceRecord], immutable.Seq.empty[ResourceRecord]))
case class ResolveFailedException(msg: String) extends Exception(msg)
+
Review Comment:
Remove this new line?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580768720
Lets wait a bit before merging this, I think I was wrong in what I said before about collisions and we do need to handle this.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1224222412
##########
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:
I don't know if the code that @Claudenw provided is meant to be threadsafe, it was just an assumption based on the fact that the original code had `volatile` in it.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #371: Generate random DNS id's
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1230232215
##########
actor/src/main/scala/org/apache/pekko/util/UniqueRandomShortProvider.scala:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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._
+
+ // These methods are volatile so that they stay within cache
+ @volatile private var index = 0L
Review Comment:
I think @Claudenw agreed that the volatiles should go
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#discussion_r1230234594
##########
actor/src/main/scala/org/apache/pekko/util/UniqueRandomShortProvider.scala:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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._
+
+ // These methods are volatile so that they stay within cache
+ @volatile private var index = 0L
Review Comment:
So I just spoke with him and he said that he did put volatile there deliberately, its a trick to keep the cache warm (see https://github.com/apache/incubator-pekko/pull/371#discussion_r1228105897).
I will do another set of benchmarks without volatile to see if it makes a difference, although its so fast that JMH is failing to display the numbers with enough precision.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #371: Generate random DNS id's
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #371:
URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1592079440
> Isn't 10^-4 a small number meaning poor performance?
Ah its in throughput mode, in this case something seems to be up.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org