Second round of changing null checks to !! #65108

stephentoub · 2022-02-09T22:09:04Z

Round 2, following #64720. This should be most of what remains, with the exception of a few older libraries (e.g. System.Management) I haven't looked at yet.

ghost · 2022-02-09T22:09:15Z

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

Issue Details

Round 2, following #64720. This should be most of what remains, with the exception of a few older libraries (e.g. System.Management) I haven't looked at yet.

Author:	stephentoub
Assignees:	stephentoub
Labels:	`area-Meta`
Milestone:	-

EgorBo · 2022-02-09T22:19:12Z

Unrelated to this PR, but I wonder if we should add some love to this feature in JIT, because it looks simple and light-weight and users might start [ab]using it a lot just in sake of safety.
E.g.

https://sharplab.io/#v2:EYLgtghglgdgNAFxBAzmAPgAQEwEYCwAUJgMwAEOZAwmQN5FmMW4BsFALGQGID2PAFD2AArAKYBjBGR64AhLLjSREqT2zzFQsZOkl5ASgZN6hJmQC+Rc0A==

I check 3 arguments for null here and it emits 3 call throw; ret3. I wonder if we should try to merge calls to "noreturn" same methods. cc @dotnet/jit-contrib - not for performance, just for size (e.g. R2R)

NOTE: the arguments are different.

E.g.:

-L0015: mov ecx, 1
-L001a: mov edx, 0x1120c030
-L001f: call 0x71f187c0
-L0024: mov ecx, eax
-L0026: call dword ptr [0x1120c938]
-L002c: int3
-L002d: mov ecx, 7
-L0032: mov edx, 0x1120c030
-L0037: call 0x71f187c0
-L003c: mov ecx, eax
-L003e: call dword ptr [0x1120c938]
-L0044: int3
-L0045: mov ecx, 0xd
-L004a: mov edx, 0x1120c030
-L004f: call 0x71f187c0
-L0054: mov ecx, eax
-L0056: call dword ptr [0x1120c938]
-L005c: int3
+L0015: mov ecx, 1
+       jmp short L004a
+L002d: mov ecx, 7
+       jmp short L004a
+L0045: mov ecx, 0xd
+L004a: mov edx, 0x1120c030
+L004f: call 0x71f187c0
+L0054: mov ecx, eax
+L0056: call dword ptr [0x1120c938]
+L005c: int3

stephentoub · 2022-02-09T22:33:35Z

I wonder if we should add some love to this feature in JIT

Yes? 😄

MihaZupan · 2022-02-09T22:40:14Z

Would it help if the compiler generated a helper method per argument name instead of passing it as an argument? Or at least in cases where the argument name repeated a lot?

SharpLab

EgorBo · 2022-02-09T22:42:49Z

Would it help if the compiler generated a helper method per argument name instead of passing it as an argument? Or at least in cases where the argument name repeated a lot?

SharpLab

Yes, but it increases .dll size

stephentoub · 2022-02-09T22:43:06Z

Would it help if the compiler generated a helper method per argument name instead of passing it as an argument

Every method adds non-trivial metadata overhead. If we were going to go such a route, I expect we'd prefer the ExceptionArgument route we use manually in some assemblies like Corelib, but we explicitly discussed and decided against that route for the general feature.

MihaZupan · 2022-02-09T22:50:14Z

I suppose it is called <PrivateImplementationDetails> for a reason 😄

I would expect there is a cut-off at some point where it's a win even for size to special-case e.g. "value" after it repeats 100 times.

stephentoub · 2022-02-10T20:51:29Z

@RikkiGibson, @jaredpar, just FYI... we're getting close to being done with the rollout... after this one I think we're mostly in the weeds.

AndyAyersMS · 2022-02-10T21:13:40Z

I wonder if we should add some love to this feature in JIT

@EgorBo fgTailMergeThrows does some of what we would need here, but it appears the pattern match needed is quite a bit more involved and may span multiple statements.

src/libraries/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/BinderHelper.cs

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.cs

src/libraries/System.Private.CoreLib/src/System/Collections/Hashtable.cs

bartonjs · 2022-02-10T21:59:05Z

src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs

            private readonly Stream _stream;

-            internal SyncStream(Stream stream) => _stream = stream ?? throw new ArgumentNullException(nameof(stream));
+            internal SyncStream(Stream stream!!) => _stream = stream;


source.dot.net claims that there's only one caller of this ctor (lines 722-723) and it is already guaranteeing non-null.

If we wanted, we could probably change this to Debug.Assert.

Makes sense. I'll change that up subsequently (unless I need to restart CI, in which case I'll do it here).

src/libraries/System.Private.DataContractSerialization/src/System/Text/Base64Encoding.cs

src/libraries/System.Private.DataContractSerialization/src/System/Xml/XmlBaseReader.cs

src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XComment.cs

...s/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/BlobEncoders.cs

stephentoub · 2022-02-11T01:39:55Z

Thanks for reviewing, @bartonjs!

ghost assigned stephentoub Feb 9, 2022

ghost added the area-Meta label Feb 9, 2022

stephentoub force-pushed the morenullchecks branch from 5429885 to 62b32bf Compare February 9, 2022 22:33

Second round of changing null checks to !!

db4f626

stephentoub force-pushed the morenullchecks branch from 62b32bf to db4f626 Compare February 10, 2022 01:11