Skip to content

Commit

Permalink
Avoid weak gamma values in Hedgehog.Internal.Seed
Browse files Browse the repository at this point in the history
See also:
- http://www.pcg-random.org/posts/bugs-in-splitmix.html
- hedgehogqa#191
- http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java
- http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/util/SplittableRandom.java

In http://www.pcg-random.org/posts/bugs-in-splitmix.html, most of the output is horribly formatted.

So I went on and formatted the important part of the output from that article, where it highlights the bug:

>```
>Prelude System.Random.SplitMix> map mkSMGen
> [ 0x61c8864680b583eb
> , 0xf8364607e9c949bd
> , 0x88e48f4fcc823718
> , 0x7f83ab8da2e71dd1
> , 0x7957d809e827ff4c
> , 0xf8d059aee4c53639
> , 0x9cd9f015db4e58b7
> , 0xf4077b0dbebc73c0
> , 0x305cb877109d0686
> , 0x359e58eeafebd527
> , 0xbeb721c511b0da6d
> , 0x86466fd0fcc363a6
> , 0xefee3e7b93db3075
> , 0x79629ee76aa83059
> , 0x05d507d05e785673
> , 0x76442b62dddf926c ]
>
> [ SMGen {_seed = 16269636050940713694, _gamma =  1}
> , SMGen {_seed = 11331281847540010644, _gamma =  1}
> , SMGen {_seed = 5619286150931196027 , _gamma =  1}
> , SMGen {_seed = 10281499533683189089, _gamma =  1}
> , SMGen {_seed = 17485363145453140727, _gamma =  7}
> , SMGen {_seed = 17937441790592959953, _gamma =  7}
> , SMGen {_seed = 15529190628949695919, _gamma =  7}
> , SMGen {_seed = 6957216782316743206 , _gamma =  7}
> , SMGen {_seed = 13022414457527836317, _gamma = 15}
> , SMGen {_seed = 2127073997383595602 , _gamma = 15}
> , SMGen {_seed = 7685868475082380828 , _gamma = 15}
> , SMGen {_seed = 6396586367974718702 , _gamma = 15}
> , SMGen {_seed = 13838712901582905874, _gamma = 31}
> , SMGen {_seed = 15464227283579875280, _gamma = 31}
> , SMGen {_seed = 17142043965827948305, _gamma = 31}
> , SMGen {_seed = 10266498716653543189, _gamma = 31} ]
>Prelude System.Random.SplitMix>
>```

It compares just *one* implementation of SplitMix, [this](https://github.com/phadej/splitmix/blob/f6895c4137b14e4752b0575c178199201eb41810/src/System/Random/SplitMix.hs) one, at some particular point in time.

(I can see that the author went and fixed it recently as shown from the diff-output [here](haskellari/splitmix@bd0692b) and [here](haskellari/splitmix@c6ba2f3), so this means that now *The Bug* section of http://www.pcg-random.org/posts/bugs-in-splitmix.html is outdated.)

---

Here's the exact same test with *Hedgehog.Internal.Seed*:

```
λ :t from
from :: GHC.Word.Word64 -> Seed

λ Prelude.map from
  [ 0x61c8864680b583eb
  , 0xf8364607e9c949bd
  , 0x88e48f4fcc823718
  , 0x7f83ab8da2e71dd1
  , 0x7957d809e827ff4c
  , 0xf8d059aee4c53639
  , 0x9cd9f015db4e58b7
  , 0xf4077b0dbebc73c0
  , 0x305cb877109d0686
  , 0x359e58eeafebd527
  , 0xbeb721c511b0da6d
  , 0x86466fd0fcc363a6
  , 0xefee3e7b93db3075
  , 0x79629ee76aa83059
  , 0x05d507d05e785673
  , 0x76442b62dddf926c ]

[ Seed 7046029254386353131  11400714819323198485
, Seed 17885559969949501885 11400714819323198485
, Seed 9864166656744503064  11400714819323198485
, Seed 9188376289577737681  11400714819323198485
, Seed 8743694738624347980  11400714819323198485
, Seed 17928928724259255865 11400714819323198485
, Seed 11302328716527294647 11400714819323198485
, Seed 17584158569056203712 11400714819323198485
, Seed 3484863033197266566  11400714819323198485
, Seed 3863623312507393319  11400714819323198485
, Seed 13742489918233434733 11400714819323198485
, Seed 9675543792836633510  11400714819323198485
, Seed 17288824720004427893 11400714819323198485
, Seed 8746728143070965849  11400714819323198485
, Seed 420250731748546163   11400714819323198485
, Seed 8521984098521027180  11400714819323198485 ]
λ
```

This looks weird, as I'm getting the *exact same γ* through all the seeds. If I change Hedgehog.Internal.Seed's [`from`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L108-L110) according to [Oracle's JDK8 implementation](http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java#l214):

```diff
λ :! git diff
diff --git a/hedgehog/src/Hedgehog/Internal/Seed.hs b/hedgehog/src/Hedgehog/Internal/Seed.hs
index 61cacb2..fe1a0be 100644
--- a/hedgehog/src/Hedgehog/Internal/Seed.hs
+++ b/hedgehog/src/Hedgehog/Internal/Seed.hs
@@ -98,11 +98,11 @@ random =
 -- | Create a 'Seed' using a 'Word64'.
 --
 from :: Word64 -> Seed
 from x =
-  Seed x goldenGamma
+  Seed (mix64 x) (mixGamma (x + goldenGamma))
λ
```

I can now reproduce the bug [*exactly* as shown in the article](http://www.pcg-random.org/posts/bugs-in-splitmix.html#the-bug):

```
λ Prelude.map from
  [ 0x61c8864680b583eb
  , 0xf8364607e9c949bd
  , 0x88e48f4fcc823718
  , 0x7f83ab8da2e71dd1
  , 0x7957d809e827ff4c
  , 0xf8d059aee4c53639
  , 0x9cd9f015db4e58b7
  , 0xf4077b0dbebc73c0
  , 0x305cb877109d0686
  , 0x359e58eeafebd527
  , 0xbeb721c511b0da6d
  , 0x86466fd0fcc363a6
  , 0xefee3e7b93db3075
  , 0x79629ee76aa83059
  , 0x05d507d05e785673
  , 0x76442b62dddf926c ]

[ Seed 15210016002011668638  1
, Seed 11409286845259996466  1
, Seed 1931727433621677744   1
, Seed 307741759840609752    1
, Seed 8606169619657412120   7
, Seed 13651108307767328632  7
, Seed 125750466559701114    7
, Seed 6781260234005250507   7
, Seed 15306535823716590088 15
, Seed 7344074043290227165  15
, Seed 9920554987610416076  15
, Seed 3341781972484278810  15
, Seed 12360157267739240775 31
, Seed 600595566262245170   31
, Seed 1471112649570176389  31
, Seed 8100917074368564322  31 ]
λ
```

This is actually great so far, as it points out that we got Hedgehog.Internal.Seed's [`from`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L108-L110) wrong and we can now 'fix' it according to the 'reference' source.

(At this point, I can tell that the only reliable source is the one found in Oracle's JDK8 implementation.)

---

And finally, If I go and change the glorious Hedgehog.Internal.Seed [`mixGamma`](https://github.com/hedgehogqa/haskell-hedgehog/blob/615880fcabcbd07f4c583fd9913c8a6985c16c44/hedgehog/src/Hedgehog/Internal/Seed.hs#L202) according to [Oracle's JDK8 implementation](http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/SplittableRandom.java#l214) (essentially, reverting hedgehogqa#124):

```diff
λ :! git diff
diff --git a/hedgehog/src/Hedgehog/Internal/Seed.hs b/hedgehog/src/Hedgehog/Internal/Seed.hs
index 61cacb2..5428927 100644
--- a/hedgehog/src/Hedgehog/Internal/Seed.hs
+++ b/hedgehog/src/Hedgehog/Internal/Seed.hs
@@ -100,7 +100,7 @@ random =
 --
 from :: Word64 -> Seed
 from x =
-  Seed x goldenGamma
+  Seed (mix64 x) (mixGamma (x + goldenGamma))

 -- | A predefined gamma value's needed for initializing the "root" instances of
 --   'Seed'. That is, instances not produced by splitting an already existing
@@ -192,7 +192,7 @@ mixGamma x =
     y = mix64variant13 x .|. 1
     n = popCount $ y `xor` (y `shiftR` 1)
   in
-    if n >= 24 then
+    if n < 24 then
       y `xor` 0xaaaaaaaaaaaaaaaa
     else
       y
λ
```

It 'fixes' the gamma value:

```
λ Prelude.map from
  [ 0x61c8864680b583eb
  , 0xf8364607e9c949bd
  , 0x88e48f4fcc823718
  , 0x7f83ab8da2e71dd1
  , 0x7957d809e827ff4c
  , 0xf8d059aee4c53639
  , 0x9cd9f015db4e58b7
  , 0xf4077b0dbebc73c0
  , 0x305cb877109d0686
  , 0x359e58eeafebd527
  , 0xbeb721c511b0da6d
  , 0x86466fd0fcc363a6
  , 0xefee3e7b93db3075
  , 0x79629ee76aa83059
  , 0x05d507d05e785673
  , 0x76442b62dddf926c ]

[ Seed 15210016002011668638 12297829382473034411
, Seed 11409286845259996466 12297829382473034411
, Seed 1931727433621677744  12297829382473034411
, Seed 307741759840609752   12297829382473034411
, Seed 8606169619657412120  12297829382473034413
, Seed 13651108307767328632 12297829382473034413
, Seed 125750466559701114   12297829382473034413
, Seed 6781260234005250507  12297829382473034413
, Seed 15306535823716590088 12297829382473034405
, Seed 7344074043290227165  12297829382473034405
, Seed 9920554987610416076  12297829382473034405
, Seed 3341781972484278810  12297829382473034405
, Seed 12360157267739240775 12297829382473034421
, Seed 600595566262245170   12297829382473034421
, Seed 1471112649570176389  12297829382473034421
, Seed 8100917074368564322  12297829382473034421 ]
λ
```
  • Loading branch information
moodmosaic committed May 23, 2018
1 parent 7e61d73 commit 39b15b9
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions hedgehog/src/Hedgehog/Internal/Seed.hs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ random =
--
from :: Word64 -> Seed
from x =
Seed x goldenGamma
Seed (mix64 x) (mixGamma (x + goldenGamma))

-- | A predefined gamma value's needed for initializing the "root" instances of
-- 'Seed'. That is, instances not produced by splitting an already existing
Expand Down Expand Up @@ -199,7 +199,7 @@ mixGamma x =
y = mix64variant13 x .|. 1
n = popCount $ y `xor` (y `shiftR` 1)
in
if n >= 24 then
if n < 24 then
y `xor` 0xaaaaaaaaaaaaaaaa
else
y
Expand Down

0 comments on commit 39b15b9

Please sign in to comment.