Random.Next is slow on checked runtime on ARM

[CarnaViire]

This is a follow-up to the investigation in #52031

We had a test that consistently run overtime on Linux on ARM whereas it was ok on all other platforms and architectures. Investigation shown that the culprit was calling for Random.Next in a loop (on checked runtime). Switching to Random.NextBytes changed timings dramatically.

There are two pieces of code with the timings I've measured on Ubuntu 18.04 on an ARM64 machine on Checked CoreCLR runtime:

int frameSize = 2 << 15;
byte[] message = new byte[frameSize * 10];
Random random = new(0);
for (int i = 0; i < message.Length; ++i)
{
    message[i] = (byte)random.Next(maxValue: 10);
}

Execution time: 4.8s

int frameSize = 2 << 15;
byte[] message = new byte[frameSize * 10];
new Random(0).NextBytes(message);
for (int i = 0; i < message.Length; ++i)
{
    message[i] %= 10;
}

Execution time: 0.02s

It might be beneficial to know what causes the first piece of code to run 200 times slower.

cc @danmoseley

P.S.: it may be worth mentioning that on release runtime, the first piece takes 0.03s.

[EgorBo]

I took a look at it in PerfView and the hottest method is static void AssertInRange(double result) https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Random.cs#L212-L214

because it invokes double-to-string conversion all the time (which is slow mostly because of get_CurrentCulture)

[ghost]

Tagging subscribers to this area: @tannergooding
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This is a follow-up to the investigation in #52031

We had a test that consistently run overtime on Linux on ARM whereas it was ok on all other platforms and architectures. Investigation shown that the culprit was calling for Random.Next in a loop (on checked runtime). Switching to Random.NextBytes changed timings dramatically.

There are two pieces of code with the timings I've measured on Ubuntu 18.04 on an ARM64 machine on Checked CoreCLR runtime:

int frameSize = 2 << 15;
byte[] message = new byte[frameSize * 10];
Random random = new(0);
for (int i = 0; i < message.Length; ++i)
{
    message[i] = (byte)random.Next(maxValue: 10);
}

Execution time: 4.8s

int frameSize = 2 << 15;
byte[] message = new byte[frameSize * 10];
new Random(0).NextBytes(message);
for (int i = 0; i < message.Length; ++i)
{
    message[i] %= 10;
}

Execution time: 0.02s

It might be beneficial to know what causes the first piece of code to run 200 times slower.

cc @danmoseley

P.S.: it may be worth mentioning that on release runtime, the first piece takes 0.03s.

Author:	CarnaViire
Assignees:	-
Labels:	arch-arm32, area-CodeGen-coreclr, area-System.Runtime, untriaged
Milestone:	-

[zlatanov]

I took a look at it in PerfView and the hottest method is static void AssertInRange(double result)

Is checked runtime basically a DEBUG build? If that was the case, wouldn't this show up in the other environments as well?

[EgorBo]

I took a look at it in PerfView and the hottest method is static void AssertInRange(double result)

Is checked runtime basically a DEBUG build?

In the context of CoreLib I'd say it's Release with Asserts 🙂

[jkotas]

If that was the case, wouldn't this show up in the other environments as well?

I bet that it does. The other environments are generally faster and so the slow-down was not enough to make the test time out.

[danmoseley]

Is this a case that the new string formatting pattern would improve in future?

[jkotas]

Yes, it would improve it somewhat if we added Debug.Assert overloads that accept the new builders.

It would still be many times slower than outlining the condition manually like what Egor has done in #52918

[EgorBo]

In theory, we could introduce a new Debug.Assert API with a Func<string> so we could have:

Debug.Assert(result >= 0.0 && result < 1.0f, () => $"Expected 0.0 <= {result} < 1.0");

https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8B6YgAgBkBLMGAO1xgBMyMJWALGMgOgDkAogBUyAMQgBXOk2wYqEOmSlMYUMgu7Qy+aN2wBzWDHz0MuHgFgAUKTJCuvQSPHLZ8xQBsa9Rrk5U/ADMqD242MgBPSSVpVU5uAFkASREvWgYYSxtia2sAAQAmAEZ8gAYyPKKeABEqAzoIXDkwXABuMoqqgCUpORMeAGEIfAAHENUAZVUAN2823Ks8gGYKgs6AdmsAb2syPbIAbQSYDA4IJiTRjwAKY9Pzy5GPAHkRuQVzPggkui86Kjo+gAlABdXb7A5DaRUd50bA3ABE1QEACEAKoAcQRoPBe0qSAqKDICQiAEFcIwoBhrsAIBAPGRINI0BUAKwAHkqpQAfGRsGBYUDcWQdlZ9uKyFRAmRrgBCJlMIViiV7UUqlXVGDACT6HiibAha782HXIFA9rKlUAX2FNqswvxhLsMCa1yYkmAoTICCV4rV6pJ5Mp1IQZF55QAZBHvWR2WQiizTWQALy8gAkCK2CCtsbIpWxFvFdrtQA==

[danmoseley]

Just curious, is there a reason codegen can't special case Debug.Assert(condition, message) such that condition is evaluated before the message when the message is a template string? The implementation of course is very simple and tests the condition before using the message. That would benefit all existing code -- oh I guess it can't because the template string could have side effects (eg property gets)

[EgorBo]

Just curious, is there a reason codegen can't special case Debug.Assert(condition, message) such that condition is evaluated before the message when the message is a template string? The implementation of course is very simple and tests the condition before using the message. That would benefit all existing code -- oh I guess it can't because the template string could have side effects (eg property gets)

That would require two things:

1. Make Debug.Assert inlineable so we can propagate the string argument under the condition
2. Tell the JIT that it should always propagate it - it doesn't do it in general because of potential side effects and evaluates all the arguments before the call. This is currently not easy to do (we only propagate simple invariant arguments and don't have infrastructure to do it for more complicated trees - @AndyAyersMS had a prototype RyuJIT: IsValueType and IsAssignableFrom optimizations aren't inlining friendly. #40381 (comment))

[stephentoub]

Yes, it would improve it somewhat if we added Debug.Assert overloads that accept the new builders. It would still be many times slower than outlining the condition manually like what Egor has done in #52918

We could achieve essentially the same thing if we added overloads that took a special DebugAssertInterpolatedStringHandler. It would be preferred by the compiler, and we'd write it to short-circuit based on the bool condition. Thus when you wrote:

Debug.Assert(condition, $"{a} != {b}");

The compiler would generate something like:

var handler = new DebugAssertInterpolatedStringHandler(2, 4, condition, out bool success);
_ = success &&
    handler.AppendFormatted(a) &&
    handler.AppendLiteral(" != ") &&
    handler.AppendFormatted(b);
Debug.Assert(condition,  handler);

It would be a little slower than manually outlining, but not much, and would automatically apply to all asserts of this form. And would be achieved entirely in library; no additional compiler or JIT work required.

[danmoseley]

@stephentoub that's pretty appealing (seems it would have saved an investigation and PR here) -- do you plan to open an issue? (when you're back..)

[stephentoub]

I opened #53211.