-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Avoid most of the (small) regressions for seeded and derived Random instances #57530
Conversation
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. |
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsFixes #57272 When we introduced the new Random algorithm, we did so by factoring the old algorithm out into an implementation strategy class that is instantiated for all use other than Additionally, we introduced NextInt64 in this release, which means we can still change the algorithm it uses / what derived methods it calls. This changes the implementation to make 3 calls instead of 8. I also experimented but ultimately decided against the proposal in the cited issue to lazily-initialize the array for a derived type the first time it uses the state. The lazy initialization was measurable, and most derived types I found do end up using one of the base methods (even though obviously it could be used purely as an abstraction). We can reconsider in the future.
public class RandomSeed : TestBase
{
public override Random Create() => new Random(42);
}
public class RandomDerived : TestBase
{
public override Random Create() => new DerivedRandom();
private sealed class DerivedRandom : Random { }
}
public abstract class TestBase
{
private byte[] _buffer = new byte[10_000];
private Random _random;
public abstract Random Create();
[GlobalSetup]
public void Setup() => _random = Create();
[Benchmark]
public Random Ctor() => Create();
[Benchmark]
public int Next() => _random.Next();
[Benchmark]
public void NextMax() => _random.Next(42);
[Benchmark]
public void NextMinMax() => _random.Next(0, 42);
[Benchmark]
public void NextInt64() => _random.NextInt64();
[Benchmark]
public void NextInt64Max() => _random.NextInt64(42);
[Benchmark]
public void NextInt64MinMax() => _random.NextInt64(0, 42);
[Benchmark]
public void NextSingle() => _random.NextSingle();
[Benchmark]
public void NextDouble() => _random.NextDouble();
[Benchmark]
public void NextBytesArray() => _random.NextBytes(_buffer);
[Benchmark]
public void NextBytesSpan() => _random.NextBytes((Span<byte>)_buffer);
} I'm not currently sure why
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
When we introduced the new Random algorithm, we did so by factoring the old algorithm out into an implementation strategy class that is instantiated for all use other than `new Random()`. This ends up penalizing other uses (providing a seed and/or deriving from Random) by adding more virtual dispatch than is strictly necessary, in particular for `new Random(seed)`. This PR negates most of that (expected) regression by splitting the compat implementation in two, one class for `new Random(seed)` and one for `new DerivedRandom()`/`new DerivedRandom(seed)`; the former no longer needs to make virtual calls back out to the parent type. The former is also one that a consumer can't really do anything to improve, whereas in the derived case, the derivation may override to provide a more optimal implementation.
We haven't shipped this yet, so we can change its implementation to make 3 calls instead of 8 and to delegate to a different overload of Next.
f291c00
to
c146a08
Compare
Fixes #57272
cc: @GrabYourPitchforks, @tannergooding
When we introduced the new Random algorithm, we did so by factoring the old algorithm out into an implementation strategy class that is instantiated for all use other than
new Random()
. This ends up penalizing other uses (providing a seed and/or deriving from Random) by adding more virtual dispatch than is strictly necessary, in particular fornew Random(seed)
. This PR negates most but not all of that (expected and small) regression by splitting the compat implementation in two, one class fornew Random(seed)
and one fornew DerivedRandom()
/new DerivedRandom(seed)
; the former no longer needs to make virtual calls back out to the parent type. The former is also one that a consumer can't really do anything to improve, whereas in the derived case, the derivation may override to provide a more optimal implementation.Additionally, we introduced NextInt64 in this release, which means we can still change the algorithm it uses / what derived methods it calls. This changes the implementation to make 3 calls instead of 8.
I also experimented but ultimately decided against the proposal in the cited issue to lazily-initialize the array for a derived type the first time it uses the state. The lazy initialization was measurable, and most derived types I found do end up using one of the base methods (even though obviously it could be used purely as an abstraction). We can reconsider in the future.
I'm not currently sure why
NextBytesArray
gets a little slower... that'll need more investigation.