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

std.rand: Refactor Random interface #10045

Merged
merged 1 commit into from
Oct 27, 2021
Merged

Conversation

ominitay
Copy link
Contributor

These changes have been made to fix the performance issues reported in issue #10037. The Random interface was implemented in such a way that it causes significant slowdown when calling the fill function of the rng used. (reasoning is commented below the linked issue).

The Random interface is no longer stored in a field of the rng, and is instead returned by the child function random() of the rng. This avoids the performance issues caused by the interface. These changes are breaking, and will break any code outside of Zig which depends upon it. There is no conceivable way to avoid this.

cc: @SpexGuy
resolves #10037

These changes have been made to resolve issue ziglang#10037. The `Random`
interface was implemented in such a way that causes significant slowdown
when calling the `fill` function of the rng used.

The `Random` interface is no longer stored in a field of the rng, and is
instead returned by the child function `random()` of the rng. This
avoids the performance issues caused by the interface.
@SpexGuy SpexGuy added breaking Implementing this issue could cause existing code to no longer compile or have different behavior. standard library This issue involves writing Zig code for the standard library. labels Oct 27, 2021
@SpexGuy
Copy link
Contributor

SpexGuy commented Oct 27, 2021

Looks good to me, thanks for making these changes!

@ominitay
Copy link
Contributor Author

Thanks :) I'm glad I could help

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Happy to merge this, just need to make sure all the tests are passing.

Thanks @ominitay!

@ominitay
Copy link
Contributor Author

Just to indicate the significance of this performance difference, here are the results of benchmarking generating 100,000,000 i32s
benchmark

@andrewrk
Copy link
Member

Due to #10008 I'm about to do a full test suite run locally, so I'll merge and then fix any failures that I find.

@andrewrk andrewrk merged commit c1a5ff3 into ziglang:master Oct 27, 2021
@ominitay ominitay deleted the fix-random branch October 27, 2021 20:14
@leecannon leecannon mentioned this pull request Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Severe Performance Issue With std.rand.Random
3 participants