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

Lazily initialize base state for Random-derived type #81627

Merged
merged 1 commit into from Feb 14, 2023

Conversation

stephentoub
Copy link
Member

When the legacy Random algorithm is used, which happens when a seed is supplied or when a type derived from Random is used, the state for the algorithm is initialized, including an int[56] that gets allocated. For a Random-derived type that overrides all of the base methods, however, that state is never used. We can create it lazily rather than at ctor time and avoid those base costs if they're never used.

(I'd previously made several attempts at this, but each ended up with regressions up to 15%. Any regressions here appear to be within the noise, and even if they manifest, would only be limited to the case of Random-derived types that don't supply their own implementations.)

Fixes #60549

When the legacy Random algorithm is used, which happens when a seed is supplied or when a type derived from Random is used, the state for the algorithm is initialized, including an int[56] that gets allocated.  For a Random-derived type that overrides all of the base methods, however, that state is never used.  We can create it lazily rather than at ctor time and avoid those base costs if they're never used.

(I'd previously made several attempts at this, but each ended up with regressions up to 15%.  Any regressions here appear to be within the noise, and even if they manifest, would only be limited to the case of Random-derived types that don't supply their own implementations.)
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost assigned stephentoub Feb 4, 2023
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

I am not sure if an extra int[56] allocation is worth the proposed code complexity. Do we have any evidence that shows that it's a problem in any real-world app? I would expect users to reuse a Random instance (that is why we have Random.Shared) and this particular allocation to never be on a hot path.

@stephentoub
Copy link
Member Author

stephentoub commented Feb 13, 2023

I would expect users to reuse a Random instance (that is why we have Random.Shared) and this particular allocation to never be on a hot path.

Random is both an abstraction and a base implementation. It's like saying that implementing an interface brings with it an unavoidable 240 byte array allocation each time it's instantiated. I don't think we're in a position to say when someone chooses to allocate an instance of their prng, and we're currently saying that doing so always requires an additional sizeable array allocation.

@stephentoub stephentoub merged commit fc1ce86 into dotnet:main Feb 14, 2023
@stephentoub stephentoub deleted the randomalloc branch February 14, 2023 01:43
@koszeggy
Copy link

I am not sure if an extra int[56] allocation is worth the proposed code complexity.

I'm glad it was fixed this way. My real concern wasn't even the allocated memory but the cost of the (unnecessary) initialization.

Even if new Random().Next() has always been a bad practice (and it didn't even work correctly in .NET Framework) I think it's a good thing that now new MyDerivedRandom().Next() has a chance to be as performant as doing the same with the non-derived Random.

Do we have any evidence that shows that it's a problem in any real-world app?

Apart from the (bad) example above a better one can be an implementation like this one, which allocates a new instance from every thread it's accessed from. Since now we have the Random.Shared property as well, maybe my version is not that relevant anymore but it still provides a thread-safe wrapper for any Random implementations whereas Shared is tied to a specific algorithm.

@dotnet dotnet locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[.NET 6]A derived Random class still allocates possibly unnecessary memory
4 participants