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.

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

Use exponential distribution for P2P session timeouts #1244

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

Conversation

larskuhtz
Copy link
Contributor

Along the way, fix implementation of timeSpanToMicros and add roundtrip tests for time span conversions.

Copy link
Contributor

@gregorycollins gregorycollins left a comment

Choose a reason for hiding this comment

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

In general I obviously approve -- see my suggestion re swapping System.Random out in preference for mwc-random (way faster)

timeSpanToSeconds :: Integral a => TimeSpan a -> Seconds
timeSpanToSeconds (TimeSpan ms) = Seconds . int $ ms `div` 1000000
timeSpanToSeconds (TimeSpan us) = Seconds . int $ us `div` 1000000
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're here -- maybe make these functions strict?

--
sessionTimeout :: TimeSpan Int -> IO (TimeSpan Int)
sessionTimeout expected = do
x <- exponential (1 / us) :: IO Double
Copy link
Contributor

Choose a reason for hiding this comment

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

Use System.Random.MWC instead, it's better in basically every way -- including already shipping with a truncated exponential distribution. I used it for exactly this in https://github.com/kadena-io/chainweb-node/pull/1254/files, you could crib the code from there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to use it. However, I run into compatibility issues with our rather old nix snapshot. I don't remember the details. However, this implementation should be the same than what is used in mac-random. Also, I think, modern versions of the random package have much better performance than previous versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lars -- mwc-random was already in the deps list?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants