ghost
commented
1 year ago Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch See info in area-owners.md if you want to be subscribed.
Issue Details
### Description I was cleaning up some code, and made a simple change in my repo, something like this: Before: ```cs if (!value && _state != null && curTime - _state._nextTime > 0) { _state._nextTime = curTime; } ``` After: ```cs if (!value && curTime - _state?._nextTime > 0) { _state._nextTime = curTime; } ``` I happen to have the lowered C# viewer open, and noticed the new code was not that great. So I benchmarked it to see if it mattered for performance, and it looks like the optional chaining has a significant different in performance. I guess I am wondering if there is a way to add in logic to identify a null check and pull it out, as the developer would in older C# code. ### Configuration * .NET 7 * Windows 11 (x64) - I can also run this on arm64 macos ventura if necessary Here is the Benchmark.NET example code I came up with. I would love to refine this for the sake of tracking down if this is worth fixing: ```cs using BenchmarkDotNet.Attributes; using BenchmarkDotNet.Jobs; namespace Benchmarks; [SimpleJob(RuntimeMoniker.Net70)] public class BenchmarkNullable { private static SomeObject _someObject = new(); private static long _someConstant = 500; private static long _anotherVariable; private static bool _someOtherValue = false; [Benchmark] public long BenchmarkNullableCode() { if (!_someOtherValue && _someConstant - _someObject?._someValue > 0) { _anotherVariable = _someObject._someValue; } return _anotherVariable; } [Benchmark] public long BenchmarkOldCode() { if (!_someOtherValue && _someObject != null && _someConstant - _someObject._someValue > 0) { _anotherVariable = _someObject._someValue; } return _anotherVariable; } internal class SomeObject { internal long _someValue = 100; } } ``` ### Regression? I am not sure if this is a regression per-se, or a naive unfurling of the syntax sugar and warrants investigation? At the least, the community should be made aware to not "clean" their code in this way I suppose. ### Data #### .NET 7 ```cs | Method | Mean | Error | StdDev | Median | |---------------------- |----------:|----------:|----------:|----------:| | BenchmarkNullableCode | 0.4463 ns | 0.0307 ns | 0.0287 ns | 0.4429 ns | | BenchmarkOldCode | 0.0025 ns | 0.0041 ns | 0.0038 ns | 0.0000 ns | ``` ### .NET 8 I ran the same benchmark with .NET 8, and the results look good. So maybe this is a moot point? Not using a nullable is 10x slower, but using a nullable has the same performance. Is this a potential regression in the optimization of the old code? ```cs | Method | Mean | Error | StdDev | |---------------------- |----------:|----------:|----------:| | BenchmarkNullableCode | 0.1011 ns | 0.0122 ns | 0.0102 ns | | BenchmarkOldCode | 0.1090 ns | 0.0143 ns | 0.0127 ns | ``` ### Analysis The lowered code for the null conditional looks like this: ```cs public long BenchmarkNullableCode() { if (!BenchmarkNullable._someOtherValue) { BenchmarkNullable.SomeObject someObject = BenchmarkNullable._someObject; long? nullable1; if (someObject == null) nullable1 = new long?(); else nullable1 = new long?(someObject._someValue); long? nullable2 = nullable1; long someConstant = BenchmarkNullable._someConstant; long? nullable3; if (!nullable2.HasValue) nullable3 = new long?(); else nullable3 = new long?(nullable2.GetValueOrDefault() - someConstant); long? nullable4 = nullable3; long num = 0; if (nullable4.GetValueOrDefault() > num & nullable4.HasValue) BenchmarkNullable._anotherVariable = BenchmarkNullable._someObject._someValue; } return BenchmarkNullable._anotherVariable; } ``` This is actually better than the lowered code I saw from my repo: ```cs int num1; if (!value) { long num2 = curTime; NetState netState = this._state; long? nullable1; if (netState == null) nullable1 = new long?(); else nullable1 = new long?(netState._nextTime); long? nullable2 = nullable1; long? nullable3; if (!nullable2.HasValue) nullable3 = new long?(); else nullable3 = new long?(num2 - nullable2.GetValueOrDefault()); long? nullable4 = nullable3; long num3 = 0; num1 = nullable4.GetValueOrDefault() > num3 & nullable4.HasValue ? 1 : 0; } else num1 = 0; if (num1 == 0) return; this._state._nextTime = curTime; ``` Note the ternary: `num1 = nullable4.GetValueOrDefault() > num3 & nullable4.HasValue ? 1 : 0;` - Not sure why that is was necessary. From what I can tell, it is simply really verbose and not efficient.
| Author: | kamronbatman |
|---|---|
| Assignees: | - |
| Labels: | `tenet-performance`, `area-CodeGen-coreclr`, `untriaged`, `needs-area-label` |
| Milestone: | - |
huoyaoyuan
kamronbatman
jakobbotsch
jnyrup
AndyAyersMS
Description
I was cleaning up some code, and made a simple change in my repo, something like this:
Before:
After:
I happen to have the lowered C# viewer open, and noticed the new code was not that great. So I benchmarked it to see if it mattered for performance, and it looks like the optional chaining has a significant different in performance. I guess I am wondering if there is a way to add in logic to identify a null check and pull it out, as the developer would in older C# code.
Configuration
Here is the Benchmark.NET example code I came up with. I would love to refine this for the sake of tracking down if this is worth fixing:
Regression?
I am not sure if this is a regression per-se, or a naive unfurling of the syntax sugar and warrants investigation? At the least, the community should be made aware to not "clean" their code in this way I suppose.
Data
Benchmarks for .NET 7
Disassembly Diagnoser for .NET 7
.NET 7.0.12 (7.0.1223.47720), X64 RyuJIT AVX2
.NET 7.0.12 (7.0.1223.47720), X64 RyuJIT AVX2
Benchmarks for .NET 8
I ran the same benchmark with .NET 8, and the results look good. So maybe this is a moot point?
Disassembly Diagnoser for .NET 8
.NET 8.0.0 (8.0.23.47906), X64 RyuJIT AVX2
.NET 8.0.0 (8.0.23.47906), X64 RyuJIT AVX2
Analysis
The lowered code for the null conditional looks like this:
This is actually better than the lowered code I saw from my repo:
Note the ternary:
num1 = nullable4.GetValueOrDefault() > num3 & nullable4.HasValue ? 1 : 0;- Not sure why that was necessary.From what I can tell, it is simply really verbose and not efficient.