Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Interface to performance #1

Open
wants to merge 231 commits into
base: master
Choose a base branch
from
Open

Interface to performance #1

wants to merge 231 commits into from

Conversation

lehins
Copy link
Collaborator

@lehins lehins commented Feb 12, 2020

Proposal

@idontgetoutmuch, @cartazio and @curiousleo since you guys asked me about this, here is what I suggest.

CC @ekmett (you were last one to release random) and @phadej (you have the best candidate for default pure random number generator for Haskell)

These are the initial changes I propose to the random package. At a high level it contains:

  • ability to provide efficient generation of all major types: Word8 .. Double, which are further used by Random class. (Method names are self explanatory: nextWord8, nextWord16 ...)
  • Addition of RandomPrimGen and extra two methods to Random class: randomPrim and randomPrimR, which would allow us to use the same interface for stateful random number generators like mwc-random and pcg-random
  • also added bitmaskWithRejection which @phadej kindly mentioned on reddit and implemented in splitmix

Benefits:

What are the benefits of this approach:

  • It is backwards compatible and breakage at the API level is non-existent. Values generated will be different for the same generator as before, therefore it probably will have to be considered as a breaking change. But as far as users of random are concerned, nothing will have to be done on their part to benefit from this, which is great, because that is a lot of code!
  • Enormous performance improvement. Just consider how much energy we can save in the world by making CI for all those Haskell projects using QuickCheck run faster.
  • As mentioned earlier, addition of RandomPrimGen will allow us to use unified API for stateful and pure RNGs
  • Current RNG libraries can be updated at will, since they will not break with this change

Drawbacks

  • All libraries that implement RNGs: splitmix, mersenne-random-pure64, mwc-random, pcg-random, etc. will require a small non-breaking change added to them. For pure generators (if they want performance) implementation of new Random class functions, while for stateful addition of instances for RandomPrimGen class. Important part is: none of them will break if they don't receive any changes.
  • The work that @cartazio have done so far probably will not be compatible

Performance

A bit of comparison on why this is important. Here is a link with current performance of

random :: (Random a, RandomGen g) => g -> (a, g)

restricted to a :: Word64 for all libraries that have RandomGen instance: https://alexey.kuleshevi.ch/assets/iframes/2019-12-21-random-benchmarks/random64.html
and here is what it can be if we go the route I suggest. The only library I changed to use the native generator for Word64 was splitmix (64bit) version:
FireShot Capture 025 - criterion report -

Here is what I had to do to splitmix package to be able to achieve such affect: lehins/splitmix@928b9a1

TODO:

  • Functions for generation of ranges in random still needs lots of fixing, I just scratched the surface.
  • StdGen needs replacement. It is total crap. It's old, time to get something better and faster. I nominate splitmix
  • If we choose splitmix, I'd recommend just dropping splitmix dependency on random and invert it: make random depend on splitmix. Add type StdGen = SMGen to random and call it a day.
  • Both stateful and pure RNGs libraries will need to have PRs sent their way. All of which should be simple, all of them contain the hard bits.

References

@cartazio
Copy link

cartazio commented Feb 12, 2020 via email

@idontgetoutmuch
Copy link
Owner

idontgetoutmuch commented Feb 12, 2020 •
edited
Loading

@curiousleo and I have just tested random, mwc and splitmix via smallcrush and they all pass.

Code here if you are interested in reproducing: https://github.com/idontgetoutmuch/random-playground/blob/splitmix-replacement/fromStdin.c

We are currently testing via PractRand also. We'll post the results when they are available.

See also idontgetoutmuch/random-playground#1 (comment). @curiousleo is implementing @peteroupc's suggestions.

@cartazio
Copy link

cartazio commented Feb 12, 2020 via email

idontgetoutmuch
idontgetoutmuch reviewed Feb 14, 2020
View reviewed changes
Copy link
Owner

@idontgetoutmuch idontgetoutmuch left a comment

Choose a reason for hiding this comment

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

import System.Random
import System.Random.MWC

instance RandomPrimGen Gen where
  genWord8 = uniform
  genWord16 = uniform
  genWord32 = uniform
  genWord64 = uniform
  genFloat = uniform
  genDouble = uniform

but I am not sure about performance.

I am not sure where to write this but should we have PRs like the instance above tested together and ready to update all (or at least popular) random number generator packages?

@cartazio
Copy link

cartazio commented Feb 14, 2020 via email

@lehins
Copy link
Collaborator Author

lehins commented Feb 14, 2020

@idontgetoutmuch That's right, we'd submit PRs to mwc-random and pcg-random with such instances:

I am not sure where to write this but should we have PRs like the instance above tested together and ready to update all (or at least popular) random number generator packages?

@Shimuuar what's you take on such interface and would you be opposed to having an interface like the one suggested in this RandomPrimGen for stateful RNGs? If you don't think a PR with instance RandomPrimGen Gen where could get merged into mwc-random then there would be really no point in adding such class to random package either.

@idontgetoutmuch I don't quite understand your concern here, maybe cause I don't see which part could affect the performance in a negative way.

I am not sure about performance.

@cartazio Could you elaborate on this:

Those sizes as primitive ops for the pure ones won’t actually work I think

do you mean that not all packages provide all primitives (eg, nextWord16). If that is the case, then it doesn't matter, since all of next* functions have default implementations that are base on a function that is provided by all APIs, namely next. In fact if you were to merge a PR like it is right now, none of the packages that provide instance for RandomGen would break!

@cartazio
Copy link

cartazio commented Feb 14, 2020 via email

@lehins
Copy link
Collaborator Author

lehins commented Feb 14, 2020

@cartazio Improving random was not something I wanted to do, in fact I though that this was a solved problem and we can all move on with our lives writing programs that occasionally need some randomness. My blogpost with benchmarks stirred things up a bit and it seems like there is strong desire in the community to get this thing resolved.

Could you ramp down the pace

I am in no rush at all. I just prefer to get things done instead of having discussions that drag for years.

there does NEED to be a breaking change in random

I could care less if there is breakage or not, I don't personally use random, but it is something that the rest of the community probably will care about, since there is quite a bit of code that depends on it.

Also I’ve not agreed that this is the design for next random yet.

That is why the title of this ticket is "Proposal". Looking forward to seeing your alternative approach.

the current api does not make it easy to write sampling programs that behave the same on different platforms given the same seed when using non platform independent algorithms on top

That sounds interesting. You probably have some examples of this laying around, could you please share some code snippets or a link that has 'em?

Along with a few other issues.

It sounds like you have this sort it out, but if you don't mind, could you please list those issues, so everyone has a clear idea what the problem/solution set is.

Providing a good bridge tool for spanning that breakage is / was honestly the challenge that got me into an anxious spiral that delayed releasing and iterating on that for too long.

You don't need to do it all by yourself. There are plenty of people who are willing to help.

@Shimuuar
Copy link

@Shimuuarhttps://github.com/Shimuuar what's you take on such interface and would you be opposed to having an interface like the one suggested in thisRandomPrimGen for stateful RNGs? If you don't think a PR with instance RandomPrimGen Gen where could get merged into mwc-random then there would be really no point in adding such class torandom package either.

Generally I think current state of PRNGs is deeply unsatisfactory. Each PRNG library has its own interface and only implements fraction of generators and it's impossible to reuse code. Ideally PRNGs should just implement interface and reuse all derived generators (generate numbers in range, What is general idea? Is it to modify random. Is it to create separate library?

Offhand I can say that I think Float & Double shouldn't be part of interface. They are trivially derived from uniform Word32/Word64. And there's problem: are we generating in [0,1] range? (0,1]?

I also played wit generic API for PRNGs and will try to find what did I wrote over weekends.

@idontgetoutmuch
Copy link
Owner

Is it to modify random. Is it to create separate library?

Modify random

@idontgetoutmuch
Copy link
Owner

Generally I think current state of PRNGs is deeply unsatisfactory. Each PRNG library has its own interface and only implements fraction of generators and it's impossible to reuse code. Ideally PRNGs should just implement interface and reuse all derived generators (generate numbers in range, What is general idea? Is it to modify random. Is it to create separate library?

Offhand I can say that I think Float & Double shouldn't be part of interface. They are trivially derived from uniform Word32/Word64. And there's problem: are we generating in [0,1] range? (0,1]?

I also played wit generic API for PRNGs and will try to find what did I wrote over weekends.

@Shimuuar the idea is to do something like this:

import System.Random
import qualified System.Random.MWC as MWC
import qualified System.Random.PCG as PCG
import qualified System.Random.SFMT as SFMT

instance RandomPrimGen PCG.Gen where
  genWord8 = PCG.uniform
  genWord16 = PCG.uniform
  genWord32 = PCG.uniform
  genWord64 = PCG.uniform
  genFloat = PCG.uniform
  genDouble = PCG.uniform

instance RandomPrimGen SFMT.Gen where
  genWord8 = SFMT.uniform
  genWord16 = SFMT.uniform
  genWord32 = SFMT.uniform
  genWord64 = SFMT.uniform
  genFloat = SFMT.uniform
  genDouble = SFMT.uniform

and then you have the same interface for every stateful RNG:

*Main> PCG.create >>= genDouble
0.582712623703629
*Main> MWC.create >>= genDouble
2.481036288296201e-2
*Main> SFMT.create >>= genDouble
6.1754467473337606e-2

However, I've noticed all 3 of the RNGs define a class Variate

Are you suggesting we should put the class definition in random and then just create instances of it (as those 3 packages do) rather than have the class RandomGen? That gives the same interface for all 3 RNGs and doesn't change the interface at all.

@lehins anything you want to add?

@Shimuuar
Copy link

API

I think API is not quite right. Here is my take on API. PRNGs generate uniformly distributed number in some range. Sometimes it's full range of Word32 (mwc-random), sometimes it's not (most of LCGs). For latter generation of uniformly distributed Word32/Word64 is not a primitive operation and is quite complicated! Here is API that should I think accommodate both:

class Monad m => MonadRandom m where
  -- | Generate uniformly distributed 32-bit word
  uniformWord32    :: m Word32
  -- | Generate uniformly distributed 64-bit word
  uniformWord64    :: m Word64
  -- | Generate uniformly distributed 32-bit word in range [0,n]
  uniformRWord32   :: Word32 -> m Word32
  -- | Generate uniformly distributed 32-bit word in range [0,n]
  uniformRWord64   :: Word64 -> m Word64
  • Primitives for Word8/16 are absent. Those could be derived from uniformRWord32 and I don't think there're any generators of practical use that use Word8/16 internally
  • uniformRWord32/64 is a primitive because if generator does not generate full range of Word32 implementation in terms of uniformWord32 is not efficient
  • Primitives for Float/Double are absent. As I already said definition it very simple and there're considerations whether to include 0 into range or not.
wordToFloatZ :: Word32 -> Float
wordToFloatZ x = (fromIntegral i * f_inv_32) + 0.5
  where
    i = fromIntegral x :: Int32

Another consideration is handling of PRNG's state (save/restore) and initialization. We need to provide support for both because it wouldn't be possible or at least not easy to work with different PRNGs since you'll have to switch not only PRNG but full initialization code

Also some PRNG's also support more than just generating stateful stream of random numbers. AFAIR PCG support generation of many independent streams so do counting PRNGs (random123). Do we need to accomodate such generators?

Variate class

I think this type class is sort of obvious. But it's also wrong. I strongly suspect that it was introduced in mwc-random and later copied by other two. What's wrong with it? uniform & uniformR belong to different type classes.

  • uniform says: generate all possible values with equal probability. (Word8,Word8) clearly admits instance but what about uniformR? There's no good definition
  • uniformR says generate all possible values with equal probability. Integer works just fine with this. But uniform? No way! There's infinite number of integers

This is bad design and should be abandoned.

P.S.

All in all I think we already have API that doesn't work well. We have sort-of-standard random which shouldn't be used. Lets try to do things right this time. It's true design space is very complicated here. We have PRNGs that could be implemented as pure functions, we have ones that require in place mutations!

@cartazio
Copy link

cartazio commented Feb 15, 2020 via email

@cartazio
Copy link

cartazio commented Feb 15, 2020 via email

@Shimuuar
Copy link

I think it only strengthens argument for not including floating point numbers into base API. It's not possible to include all subtle variations and it's not reasonable to expect that all instances will implement them identically

@lehins
Copy link
Collaborator Author

lehins commented Feb 15, 2020

I do not agree with this one:

Primitives for Word8/16 are absent.

We should allow a specific RNG to decide what is the most efficient way to generate 8 and 16 bits of random data. Example StdGen, it can't generate full 32 bits in one go. That being said they can all have default implementations, which means supplying either genWord32 or genWord64 will be sufficient

I don't understand the point of these: uniformRWord32 or uniformRWord64 We can decide on the best known approach on generating subranges, for example bitmaskWithRejection

With regards to floating point numbers I am leaning towards not including them for consistency with ranges. Having a single efficient implementation for all RNGs that use either 32bits or 64bits seems like a better idea

@Shimuuar
Copy link

It's for cases when generating full Word32 is not a primitive operation. LGCs for example generate numbers in range [0,p-1] where p is constant. (2^31-1 is common choice). Thus definition of unimformWord32 is quite complicated, require multiple PRNG iterations and possibly rejections therefore implementing uniformRWord32 in terms uniformWord32 will be inherently inefficient,

On one hand yes. No one in the right mind will use LGC. On other I think it sets precedent that it''s possible that generator produces something else than uniform Word32/64. And generic API that could accomodate such generators is more future proof and less likely to run into problems with some weird PRNG. In some sense uniformRWord32/64 are more fundamental. They ask to generate N distinct possibilities and uniformWord are just special cases which could be implemented more efficiently for some PRNGs

Even for PNRGs that generate full range of Word32/Word64 optimal implementation of uniformRWord32 is different different depending on width of generators' primitive. If generator uses Word32 underneath we should defined uniformRWord32 in terms of uniformWord32. It it uses Word64 we should use uniformWord64 instead

@cartazio
Copy link

cartazio commented Feb 15, 2020 via email

@idontgetoutmuch
Copy link
Owner

Several fail big crush. I’ll dig up the old code next week or so. Lots of work and personal commitments this week/month to juggle.

The current random fails pretty much everything; on the other hand splitmix passes big crush: https://github.com/tweag/random-quality/tree/master/results.

It should be easy enough to verify the results using the repo.

All RNGs will fail some test for randomness. splitmix is faster than current random and is of higher quality (in the sense that it passes more tests for randomness).

@idontgetoutmuch
Copy link
Owner

Here's the code for random Float from mwc-random

mwcWordToFloat :: Word32 -> Float
mwcWordToFloat x      = (fromIntegral i * m_inv_32) + 0.5 + m_inv_33
    where m_inv_33 = 1.16415321826934814453125e-10
          m_inv_32 =  2.3283064365386962890625e-10
          i        = fromIntegral x :: Int32

similar code exists for splitmix

splitWordToFloat :: Word32 -> Float
splitWordToFloat x = fromIntegral (x `shiftR` 8) * floatUlp
  where
    floatUlp =  1.0 / fromIntegral (1 `shiftL` 24 :: Word32)

@cartazio are you saying both of these are wrong? I couldn't understand what you were driving at in the link you posted.

@idontgetoutmuch
Copy link
Owner

If the monadic interfaces, such as the combinator examples I linked, become the Norm, I think a lot of this complexity goes away.

I can't see any links. The only link you posted is https://github.com/haskell/random/blob/master/src/Data/Distribution/FloatingInterval.hs but I can't see a monadic interface in it.

@idontgetoutmuch
Copy link
Owner

idontgetoutmuch commented Feb 17, 2020 •
edited
Loading

Here's another implementation for getting random floating point numbers

https://github.com/mokus0/random-fu/blob/69a563a7b0cf444748e4b38a8bda7ada0b9acf14/random-source/src/Data/Random/Internal/Words.hs#L103

wordToFloat :: Word64 -> Float
wordToFloat x = (encodeFloat $! toInteger (x .&. 0x007fffff {- 2^23-1 -} )) $ (-23)

I think it would be convenient to put such a function in random rather than lots of packages implementing it themselves but I don't feel strongly about it.

@cartazio
Copy link

cartazio commented Feb 17, 2020 via email

@Shimuuar
Copy link

I think we're discussing many things at once: set of primitive generators, hot to generate floating point, Variate type class, API at large. It becomes somewhat difficult to follow. @lehins, @curiousleo, @idontgetoutmuch would you mind if I create separate issues with summaries of this thread relevant to the issue?

I think it would be convenient to put such a function in random rather than lots of packages implementing it themselves but I don't feel strongly about it.

Completely agree. We should add functions for sampling in ranges as well.

@Shimuuar
Copy link

I finally looked through PR and I think main problem with RandomPrimGen is lack of universal API. There's simply no way to write code which work both for stateful PRNG like mwc-random and pure ones. This is a big problem since it precludes from writing generic libraries. For example there're quite a lot of code in mwc-random that could and should be generalized.

@cartazio
Copy link

cartazio commented Feb 17, 2020 via email

@idontgetoutmuch
Copy link
Owner

idontgetoutmuch commented Feb 18, 2020 •
edited
Loading

I think we're discussing many things at once: set of primitive generators, hot to generate floating point, Variate type class, API at large. It becomes somewhat difficult to follow. @lehins, @curiousleo, @idontgetoutmuch would you mind if I create separate issues with summaries of this thread relevant to the issue?

@Shimuuar That would be great :)

This was referenced Feb 18, 2020
Closed
Closed
@Shimuuar
Copy link

OK I've started.

@idontgetoutmuch
Copy link
Owner

idontgetoutmuch commented Apr 29, 2020 •
edited by curiousleo
Loading

I wanted to note which of the issues on https://github.com/haskell/random/ this (soon-to-be) PR addresses:

  1. Very low throughput haskell/random#51
  2. incorrect distribution of randomR for floating-point numbers haskell/random#53
  3. randomR could produce NaNs when the upper bound is infinity haskell/random#54 - I suggest this is a "won't fix" (*)
  4. Why does random for Float and Double produce exactly 24 or 53 bits? haskell/random#58 - this is addressed by Generate Float and Double via division #118 and Unbiased floating point number in unit interval #102.
  5. The seeds generated by split are not independent haskell/random#25

(*) unless someone can specify what the behaviour should be. Currently we have

 map (\s -> runGenState_ (mkStdGen s) (\g -> uniformRM (0.0, 1.0 / 0.0) g) :: Float) [0..9]
[Infinity,Infinity,Infinity,Infinity,Infinity,Infinity,Infinity,Infinity,Infinity,Infinity]

Maybe consistent NaNs would be better saying to the user that their request has no reasonable answer.

idontgetoutmuch and others added 26 commits April 29, 2020 11:46
@idontgetoutmuch
Check issue remains fixed
f913dcd
@idontgetoutmuch @curiousleo
Update test/Spec.hs
b72f1a5 
Co-Authored-By: Leonhard Markert <[email protected]>
@idontgetoutmuch
Respond to comments
8ca0d1e
@idontgetoutmuch
Merge pull request #117 from idontgetoutmuch/unit-tests
22a374e 
Check issue remains fixed
@curiousleo
Fix all regressions relative to v1.1 (#116)
7f91c2f 
* Make bitmask-with-rejection non-recursive
* INLINE some uniformRM implementations
@lehins
Move randomIO and randomRIO outside of Random class. Switch to …
adc3965 
…`MoandIO`
@curiousleo
Generate Float and Double via division (#118)
f05c2a7 
* Generate Float and Double via division

Coverage
========

Before:

    0.787% of representable Floats in the unit interval reached

After:

    7.874% of representable Floats in the unit interval reached

(A similar enumeration for Double is impossible, but it is very likely
that coverage is increased for Doubles too.)

Performance
===========

Before:

    pure/random/Float                        mean 331.1 μs  ( +- 21.67 μs  )
    pure/uniformR/unbounded/Float            mean 324.6 μs  ( +- 2.849 μs  )

    pure/random/Double                       mean 411.3 μs  ( +- 5.876 μs  )
    pure/uniformR/unbounded/Double           mean 416.8 μs  ( +- 41.93 μs  )

After:

    pure/random/Float                        mean 27.32 μs  ( +- 158.0 ns  )
    pure/uniformR/unbounded/Float            mean 27.37 μs  ( +- 422.0 ns  )

    pure/random/Double                       mean 27.34 μs  ( +- 303.1 ns  )
    pure/uniformR/unbounded/Double           mean 27.49 μs  ( +- 983.7 ns  )

* Floating point ranges inclusive in upper bound
@idontgetoutmuch
Merge pull request #119 from idontgetoutmuch/monad-io-global-stdgen
41863a2 
Make global StdGen to work in MonadIO
@lehins
Minor cleanups:
0058d77 
* Remove -fobject-code compilation, since Cmm was removed
* Fix example in cabal file
* Take care of some compile warnings in legacy benchmarks
@lehins
Lagacy benchmarks code formatting and cleanup. Get rid of compilation…
f1cbbb7 
… warnigns
@curiousleo
Remove unnecessary CPP uses
424ccdf
@curiousleo
Fix "default-language" warning, run cabal format
85e6b5c
@lehins
Merge pull request #120 from idontgetoutmuch/cleanup
a091bfc 
Cleanup
@curiousleo
Add stack artifacts to .gitignore
609ebc3
@curiousleo
Document uniform and uniformR
2013e25 
Docs ported from #98
@curiousleo
More self-contained docs for uniform and uniformR
4ad8a86
@curiousleo
Fix section include
07016de
@lehins
Merge pull request #121 from idontgetoutmuch/uniform-uniformr-docs
5e78a97 
Improve 'uniform' and 'uniformR' docs
@lehins
Refactor naming of mutable generators and switch to `TypeFamilyDepend…
cd5421f 
…encies` for `Frozen`
@idontgetoutmuch
Merge pull request #122 from idontgetoutmuch/refactor-mutable-gens
f2319cc 
Refactor mutable generators
@curiousleo
StdGen: constructor accessible via Internal only (#123)
9ee79a7 
Fixes haskell#59 by making 'StdGen' not
an instance of 'Read'.
@idontgetoutmuch
Ready for review
eab4f5a
@idontgetoutmuch
Incorporate feedback
d27ed91
@idontgetoutmuch
Replace dialect and order traditionally
6969cbb
@idontgetoutmuch
Merge pull request #124 from idontgetoutmuch/release-notes-new
31f9df2 
Ready for review
@curiousleo
Add 'instance UniformRange Natural' (#126)
1b07f29
@curiousleo
Copy link
Collaborator

Further issues addressed:

@lehins
Slight improvement in performance. (#125)
73f1f6e 
* Slight improvement in performance. (for small lengths it doubles the performance)

* Addition of a couple tests for ByteString generation

* Add `Eq` and `NFData` instances for `StdGen`

* Add benchmark for generation of `ShortByteString`s
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phadej phadej phadej left review comments

@Shimuuar Shimuuar Shimuuar left review comments

@curiousleo curiousleo curiousleo left review comments

@idontgetoutmuch idontgetoutmuch idontgetoutmuch left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lehins @cartazio @idontgetoutmuch @Shimuuar @curiousleo @phadej