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.

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

Remove dead code from Random #588

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@mgold
Contributor

mgold commented May 4, 2016

Previously, seeds carried around two unused functions split and range. Every seed carried around the same next function. By removing these, we slightly decrease the memory footprint of a Seed and reduce the runtime size by, I haven't measured, but a few hundred bytes.

You could go further by placing the State directly in the Seed value constructor without the use of a record, but I'm trying to be minimally invasive.

@rtfeldman

This comment has been minimized.

Show comment
Hide comment
@rtfeldman

rtfeldman May 15, 2016

Member

Assuming this works, any reason not to do type Seed = Seed State and save a record?

Member

rtfeldman commented May 15, 2016

Assuming this works, any reason not to do type Seed = Seed State and save a record?

@mgold

This comment has been minimized.

Show comment
Hide comment
@mgold

mgold May 15, 2016

Contributor

As mentioned above, none except being minimally invasive.

Evan has expressed interest in replacing the RNG with PCG so in that larger PR I can be more assertive.

Contributor

mgold commented May 15, 2016

As mentioned above, none except being minimally invasive.

Evan has expressed interest in replacing the RNG with PCG so in that larger PR I can be more assertive.

@rtfeldman

This comment has been minimized.

Show comment
Hide comment
@rtfeldman

rtfeldman May 15, 2016

Member

Gotcha. 👍 Makes sense to me!

Member

rtfeldman commented May 15, 2016

Gotcha. 👍 Makes sense to me!

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz May 20, 2016

Member

Can you link to the implementation of the alternate approach that is "more random"?

Member

evancz commented May 20, 2016

Can you link to the implementation of the alternate approach that is "more random"?

@mgold

This comment has been minimized.

Show comment
Hide comment
@mgold

mgold May 20, 2016

Contributor
type Int64
  = Int64 Int Int

type Seed
  = Seed Int64 Int64

Then this code. Also, these 64-bit arithmetic helpers.

If you're not planning to allow splittable seeds, you can eliminate one Int64 from the seed and make it a private constant. But I strongly suggest you include independent seeds because they're quite useful (for the multithreaded tests runner Richard is writing among other things). Also, although one frequently talks about splitting seeds themselves, I've found the generator of a seed to be a better abstraction. Trust me, I've spent a lot of time thinking about this so you don't have to :)

Contributor

mgold commented May 20, 2016

type Int64
  = Int64 Int Int

type Seed
  = Seed Int64 Int64

Then this code. Also, these 64-bit arithmetic helpers.

If you're not planning to allow splittable seeds, you can eliminate one Int64 from the seed and make it a private constant. But I strongly suggest you include independent seeds because they're quite useful (for the multithreaded tests runner Richard is writing among other things). Also, although one frequently talks about splitting seeds themselves, I've found the generator of a seed to be a better abstraction. Trust me, I've spent a lot of time thinking about this so you don't have to :)

@mgold

This comment has been minimized.

Show comment
Hide comment
@mgold

mgold May 20, 2016

Contributor

That (about independent seeds) being said, it's possible to incorporate the better RNG now and add independent seeds later. Don't let that prevent you from adding the RNG to a patch release.

Contributor

mgold commented May 20, 2016

That (about independent seeds) being said, it's possible to incorporate the better RNG now and add independent seeds later. Don't let that prevent you from adding the RNG to a patch release.

@evancz evancz closed this Jun 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment