dotnet
[VB] `CInt(Math.Round(doubleValue, MidPointRounding.AwayFromZero))` and `CInt` for rounding methods for `Decimal` are not optimized
Follows up: #26907
Version Used:
sharplab x64
Steps to Reproduce:
Imports System
Module Foo
Sub Method(dbl As Double, dec As Decimal)
Console.WriteLine(CInt(Math.Round(dbl, MidpointRounding.AwayFromZero)))
Console.WriteLine(CInt(Math.Truncate(dec)))
Console.WriteLine(CInt(Math.Round(dec, MidpointRounding.AwayFromZero)))
Console.WriteLine(CInt(Math.Ceiling(dec)))
end sub
end Module
A minimal repro, with source-code provided, is ideal. Using sharplab is preferred for compiler/language issues whenever possible.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AbEA3YaAmIA1AD4CSAtgA7QAuAzgAQDKAnvbTBQLABQfAWQj4ArhhiMAYhAh9G8liOCMBMWgAthACnzAMjAIJMAIhCXi0jfDDCGTNgJYUAhhgCUchV4DCEAHb0EOIAdADqUA6cADIOfjBa3mR+tFoCzhrBAEpmfvg6epYCDvg0sbTZIrmxAObBBgDuzqySUBAUAFowrW49nl7yvgFBMGER0bHxicmp6erBACpQlWDp8dZgPR68/QqDgSHhkTAxcQlJKWkZFbk6NoXFpcnX+DV1jc2tHV0Qm339e8NRkcTpNzjMMt4YA4MDVbhtetsFDBcox6Eo+Mj8CphGIYEA
Diagnostic Id:
If this is a report about a bug in an analyzer, please include the diagnostic if possible (e.g. "IDE0030").
Expected Behavior:
using System;
using System.Diagnostics;
using System.Reflection;
using System.Runtime.CompilerServices;
using Microsoft.VisualBasic.CompilerServices;
[assembly: CompilationRelaxations(8)]
[assembly: RuntimeCompatibility(WrapNonExceptionThrows = true)]
[assembly: Debuggable(DebuggableAttribute.DebuggingModes.Default | DebuggableAttribute.DebuggingModes.IgnoreSymbolStoreSequencePoints | DebuggableAttribute.DebuggingModes.EnableEditAndContinue | DebuggableAttribute.DebuggingModes.DisableOptimizations)]
[assembly: AssemblyVersion("0.0.0.0")]
[StandardModule]
internal sealed class Foo
{
public static void Method(double dbl, decimal dec)
{
Console.WriteLine(checked((int)Math.Round(dbl, MidpointRounding.AwayFromZero)));
Console.WriteLine(checked((int)Math.Truncate(dec)));
Console.WriteLine(checked((int)Math.Round(dec, MidpointRounding.AwayFromZero)));
Console.WriteLine(checked((int)Math.Ceiling(dec)));
}
}
Actual Behavior:
using System;
using System.Diagnostics;
using System.Reflection;
using System.Runtime.CompilerServices;
using Microsoft.VisualBasic.CompilerServices;
[assembly: CompilationRelaxations(8)]
[assembly: RuntimeCompatibility(WrapNonExceptionThrows = true)]
[assembly: Debuggable(DebuggableAttribute.DebuggingModes.Default | DebuggableAttribute.DebuggingModes.IgnoreSymbolStoreSequencePoints | DebuggableAttribute.DebuggingModes.EnableEditAndContinue | DebuggableAttribute.DebuggingModes.DisableOptimizations)]
[assembly: AssemblyVersion("0.0.0.0")]
[StandardModule]
internal sealed class Foo
{
public static void Method(double dbl, decimal dec)
{
Console.WriteLine(checked((int)Math.Round(Math.Round(dbl, MidpointRounding.AwayFromZero))));
Console.WriteLine(Convert.ToInt32(Math.Truncate(dec)));
Console.WriteLine(Convert.ToInt32(Math.Round(dec, MidpointRounding.AwayFromZero)));
Console.WriteLine(Convert.ToInt32(Math.Ceiling(dec)));
}
}
@tats-u Is this a behavior change from legacy (native) VB compiler? If not, then you should provide a good explanation why you expect what you expect?
@AlekseyTs
Is this a behavior change from legacy (native) VB compiler?
This changes (optimizes) generated code, but will not change the behavior of generated code. This just reduces extra and meaningless function calls.
See "Optimized floating-point to integer conversion" in https://learn.microsoft.com/en-us/dotnet/visual-basic/whats-new/#visual-basic-160
I double if you know the type conversions to integral ones in VB perform the banker's rounding unlike C#, which add extra process.
Providing a link to a general documentation page is not the same as providing a rational behind the changes that you are proposing.
This changes (optimizes) generated code, but will not change the behavior of generated code.
This should come with an explanation, a proof. For each case you are proposing to optimize.
@AlekseyTs
Providing a link to a general documentation page is not the same as providing a rational behind the changes that you are proposing.
Math.Round (also Decimal.Round for Decimal) is unneccesary and meaningless for already-rounded values. That is all. Could you tell me what I have to explain further?
https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Convert.cs,11fd4940860023d5
Convert.To**for decimals consist ofMath.Round(Decimal.Round) and the explicit cast. The former is unnecessary for already-rounded values.CInt(Math.Round(doubleValue, MidPointRounding.AwayFromZero))can be said as a bug fix or follow-up for #26948.
Could you tell me what I have to explain further?
For optimization changes like this we need to understand:
- How will the proposed change be meaningfully better? Reduction in IL size, profile of changes, etc ... are all good options here. Simply removing an unnecessary by itself is not always sufficient justification unless it comes paired with what removing that call provides. I don't seet that here.
- Analysis or demonstration of how the new code will be equivalent to the existing code. I don't see that here.
Before taking this change we'd need to lock down on both of these.
@jaredpar
- The change for
Math.Round(doubleValue, MidPointRounding.AwayFromZero))reduces IL size because an extra unneccesaryMath.Round(Double)can be omitted.
The current CInt for double is equivalent to (int)Math.Round(argument) in C#.
https://sharplab.io/#v2:C4LgTgrgdgNAJiA1AHwAICYCMBYAUKgBgAJVMAWAbj0JMwDoAZASygEcr8BmE9IgYSIBvPEVE9ew3GOm1OAHjgB7CACMANgFMAfEQDiG4AEkoABwjAAFAEoRM0ZLt2AZorAWlqzUQAeRALxEALQArHQEFD5EcgGh4ZGIAQR0mFZCto6OpJgkAOw+HBnSAL7phVm5RB7qGnQAchoA5gCGwEwAbhoAWhpgigWOJVKOpdJtTWBELGbA/noGxtPW/XYAskxwJooswABKylBwLA0A2gC6RAC265vbe9CHUA3MAM4zAccjjmsbW1C7+w8GnQACqKACiHVgn1W11+/3uRzoAEEAO5NACeADFehdur0YNCZN8bn87gdEaC8YoCUNCkRiXCyYCQYp6s1Wh1jE4WExgOiaXTRAzbgCKYoAAqKZ689oaLk8vkCwWnZYyUgATgshOkVx+IoRjxewGRajUFgu/i02scU3MJrNvj8OmtdIs2ysKxaAAs6EyLJ7gD6/d4YJcrKk/AE3X8Pd7fQCLCGLqkXYUAGRpy7+ALC0mix4sqmpwViAD8RGjwFjgfj939ceDoeTEaj7u8xZLRBARGAkA0HdENlphVS5YARAB5ADSY67RDHAHVeo8x4SrKrBkUgA
using System;
using System.Collections.Generic;
using System.Linq;
public class C {
public static void Main() {
IEnumerable<double> GetInput()
{
for(double x = -5.0; x <= 5.0; x += 0.1) {
yield return x;
}
yield return double.NegativeZero;
}
var input = GetInput();
MidpointRounding[] midpointRoundingList = [
MidpointRounding.ToEven,
MidpointRounding.AwayFromZero,
MidpointRounding.ToZero,
MidpointRounding.ToNegativeInfinity,
MidpointRounding.ToPositiveInfinity,
];
Console.WriteLine(
midpointRoundingList.All(m =>
input.All(x =>
(int)Math.Round(Math.Round(x, m)) == (int)Math.Round(x,m)
&& m == MidpointRounding.ToZero
? (int)Math.Round(Math.Round(x, m)) == (int)x
: true
)
) ? "OK" : "Wrong"
);
}
}
This outputs OK. This shows the new code is same as before.
MidpointRounding.AwayFromZero is the most convenient rounding method for human languages. Most people out of tech scene must not know the bankers' rounding (Math.Round without the second argument).
For decimal, removing extra Math.Truncate reduces the output IL size, too.
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using Perfolizer.Mathematics.SignificanceTesting;
BenchmarkRunner.Run<TruncateDecimalBench>();
// Mann-Whiteny
[StatisticalTestColumn(StatisticalTestKind.MannWhitney)]
public class TruncateDecimalBench
{
decimal[] inputs = [-1.6m, -1.5m, -1.4m, -1.3m, 1.3m, 1.4m, 1.5m, 1.6m, 4324258.452m, -438423.13575m,1482.2m, -14382.6m];
[Benchmark(Baseline = true)]
public int Traditional() => inputs.Sum(d => Convert.ToInt32(Math.Truncate(d)));
[Benchmark]
public int UseCast() => inputs.Sum(d => (int)Math.Truncate(d));
[Benchmark]
public int Simplest() => inputs.Sum(d => (int)d);
}
Removing extra Math.Truncate improves the performance drastically. Convert.ToInt32 is very slightly slower than the cast.
// * Summary *
BenchmarkDotNet v0.13.12, Windows 11 (10.0.22621.3155/22H2/2022Update/SunValley2)
13th Gen Intel Core i7-1355U, 1 CPU, 12 logical and 10 physical cores
.NET SDK 8.0.200-preview.23624.5
[Host] : .NET 8.0.1 (8.0.123.58001), X64 RyuJIT AVX2
DefaultJob : .NET 8.0.1 (8.0.123.58001), X64 RyuJIT AVX2
| Method | Mean | Error | StdDev | Ratio | MannWhitney(10%) | RatioSD |
|------------ |----------:|---------:|---------:|------:|----------------- |--------:|
| Traditional | 104.92 ns | 1.242 ns | 1.162 ns | 1.00 | Base | 0.00 |
| UseCast | 95.72 ns | 1.930 ns | 1.612 ns | 0.92 | Same | 0.02 |
| Simplest | 46.38 ns | 0.544 ns | 0.509 ns | 0.44 | Faster | 0.01 |
// * Hints *
Outliers
TruncateDecimalBench.UseCast: Default -> 3 outliers were removed (106.57 ns..131.66 ns)
TruncateDecimalBench.Simplest: Default -> 1 outlier was detected (46.99 ns)
Analysis or demonstration of how the new code will be equivalent to the existing code. I don't see that here.
It should have been proven in #26907. The logic is the same as double.
They just omit the duplicate or extra bankers' rounding for already-rounded values.
Math.Round(double, MidPointRounding)rounds after the floating point like otherMath.***methods considered in #26907.Convert.ToInt32(decimal)produces the same result as(int)if after the floating point of the given decimal value has already been rounded byMath.***methods.- The relationship between
Convert.ToInt32(bankers' rounding) and cast (truncate) in decimal is the same as that betweenMath.Round(bankers' rounding) and cast (truncate) in double.
- The relationship between
Could you suggest a more concrete instruction or requirement if you are not convinced yet?
Could you suggest a more concrete instruction or requirement if you are not convinced yet?
As I mentioned in one of your other issues, there are a number of operations in VB that are known to be less efficient than direct runtime method calls or C# equivalent. It is not a goal of VB to get them to equivalence.
Every change that we make to these code paths introduces cost: risk of unintentional changes, review cost, bug reports, etc ... It's not free. Given it's not a goal to make VB explicitly as fast as C# we need more of a rationale for taking such changes. A microbench mark isn't enough here. We need something more along the lines of a real world application where such a change would make a meaningful impact on the execution time. Even then my initial response would be "could they get the same perf increase by rewriting a small bit of their code vs. changing the compiler?"
Why have your team made a decision not to optimize for Math.Round(Double, MidPointRounding) or Decimal overloads? Is it because they are less frequent than those that have been optimized since Roslyn 16? If you have done it with a firm and solid reason, I will follow you.
I have raised this issue because of not just performance optimization but also granting consistency; do you not have to say "We have forgotten to optimize these overloads (because they have too lower priority or occur too less frequently) until now."?
It looks like it is just because "we mostly froze the VB spec, forgetting to account for these overloads." for me.
This issue is easier to be solved and less likely to cause regression than my another one thanks to related works that have been merged (#26948 & #26846).
It's worth noting that most APIs exposed in the BCL are functionally "user-defined" and the language has no integrated knowledge of them. For something like (int)Math.Round(Double, MidPointRounding), all it really sees is `convert the result of some opaque call, that it knows nothing about, to an integer".
It's also notably difficult for it to get any built-in understanding of these APIs because the implementation can change, it can be expanded to support more rounding modes, etc. Thus, the set of APIs that are intrinsically understood by the compiler tend to be explicitly small. Adding new ones can be very expensive and potentially error prone across different underlying runtime implementations.
Much of perf is put on the BCL implementation, just as is required for non-BCL code since those calls are also completely opaque. For most cases, the minor perf differences don't matter and for when they do, it will often be highlighted in an appropriate real world application profile (typically in the flame graph) at which point the application developer can submit an issue covering the real world scenario and/or workaround it in their own application by writing code that is more optimized with appropriate commentary on why it was done.
You are saying it is a bad idea to optimize the compiler a little depending on (or trusting) the BCL implementation too much.
CInt(Math.Truncate(Double)) and other ones that have already been optimized are exceptions, right?
Why have your team made a decision not to optimize for Math.Round(Double, MidPointRounding) or Decimal overloads?
Every change to the code base is a cost: cost to evaluate, code, review and maintain for decades. Particularly in established code the benefit of the change needs to outweigh the cost. The answer to why we haven't optimized this, or made many other changes to the compiler, is that we do not perceive the benefit to outweigh the cost and / or the change is prioritized below other changes.
do you not have to say
I've given our rationale for not taking this change right now and the type of data that would potentially change our opinion here.
You are saying it is a bad idea to optimize the compiler a little depending on (or trusting) the BCL implementation too much.
The compiler is very deliberate in which APIs in the BCL that it depends on for specific behavior. In most cases, as @tannergooding outlined, the compiler simply says a particular bit of code will call into methods with a specific name / pattern. It makes no guarantee what happens. LINQ is the best example here. Every time we depend on a BCL method implementation it means the BCL and compiler are firmly tied together forever. The BCL cannot semantically change implementations without breaking existing code.
The target functions are not likely to be run more than billion times and to meet your requirements. Thank you for your opinions. I will resign this improvement.
Those who want this to be improved can present high-impact evidence for us.