From 217a07c51befe97b7d4bf9b60ffafb18c5a7fc13 Mon Sep 17 00:00:00 2001
From: Dan Shechter <dans@houmus.org>
Date: Tue, 16 Jul 2019 15:38:01 +0300
Subject: [PATCH] Use throwhelper to increase change of inlining

- Add a ThrowHelper utility class that provides non-returning methods
  for throwing exceptions, this in turn helps to reduce the code size,
  there-by increasing the chances of inlining simple (getter/setter)
  methods, which has cascading CQ improvments in its own.
  (See: https://github.com/dotnet/coreclr/pull/6103)
- Convert checks in the form of `(x < 0 || x > max)` to `(uint) x > max`
  to further reduce code-size and increase inlining chances
- Update MSTest package deps
- Drop netfx.props (which I introduces in a previous PR to support
  building on linux) and replace with the now standard/canonical way of
  referencing NetFX BCL on all platforms with a <PackageReference>
- closes #588
---
 csharp/netfx.props                            | 26 ----------
 csharp/sbe-dll/ThrowHelper.cs                 | 47 +++++++++++++++++++
 csharp/sbe-dll/sbe-dll.csproj                 |  5 +-
 csharp/sbe-tests/sbe-tests.csproj             |  9 ++--
 .../generation/csharp/CSharpGenerator.java    | 26 +++++-----
 5 files changed, 66 insertions(+), 47 deletions(-)
 delete mode 100644 csharp/netfx.props
 create mode 100644 csharp/sbe-dll/ThrowHelper.cs

diff --git a/csharp/netfx.props b/csharp/netfx.props
deleted file mode 100644
index ca22ebeb7c..0000000000
--- a/csharp/netfx.props
+++ /dev/null
@@ -1,26 +0,0 @@
-<Project ToolsVersion="4.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">  
-  <PropertyGroup>
-    <!-- When compiling .NET SDK 2.0 projects targeting .NET 4.x on Mono using 'dotnet build' you -->
-    <!-- have to teach MSBuild where the Mono copy of the reference asssemblies is -->
-    <TargetIsMono Condition="$(TargetFramework.StartsWith('net4')) and '$(OS)' == 'Unix'">true</TargetIsMono>
-
-    <!-- Look in the standard install locations -->
-    <MonoBasePath Condition="'$(MonoBasePath)' == '' AND '$(TargetIsMono)' == 'true' AND EXISTS('/Library/Frameworks/Mono.framework/Versions/Current/lib/mono')">/Library/Frameworks/Mono.framework/Versions/Current/lib/mono</MonoBasePath>
-    <MonoBasePath Condition="'$(MonoBasePath)' == '' AND '$(TargetIsMono)' == 'true' AND EXISTS('/usr/lib/mono')">/usr/lib/mono</MonoBasePath>
-    <MonoBasePath Condition="'$(MonoBasePath)' == '' AND '$(TargetIsMono)' == 'true' AND EXISTS('/usr/local/lib/mono')">/usr/local/lib/mono</MonoBasePath>
-
-    <!-- If we found Mono reference assemblies, then use them -->
-    <FrameworkPathOverride Condition="'$(MonoBasePath)' != '' AND '$(TargetFramework)' == 'net45'">$(MonoBasePath)/4.5-api</FrameworkPathOverride>
-    <FrameworkPathOverride Condition="'$(MonoBasePath)' != '' AND '$(TargetFramework)' == 'net451'">$(MonoBasePath)/4.5.1-api</FrameworkPathOverride>
-    <FrameworkPathOverride Condition="'$(MonoBasePath)' != '' AND '$(TargetFramework)' == 'net452'">$(MonoBasePath)/4.5.2-api</FrameworkPathOverride>
-    <FrameworkPathOverride Condition="'$(MonoBasePath)' != '' AND '$(TargetFramework)' == 'net46'">$(MonoBasePath)/4.6-api</FrameworkPathOverride>
-    <FrameworkPathOverride Condition="'$(MonoBasePath)' != '' AND '$(TargetFramework)' == 'net461'">$(MonoBasePath)/4.6.1-api</FrameworkPathOverride>
-    <FrameworkPathOverride Condition="'$(MonoBasePath)' != '' AND '$(TargetFramework)' == 'net462'">$(MonoBasePath)/4.6.2-api</FrameworkPathOverride>
-    <FrameworkPathOverride Condition="'$(MonoBasePath)' != '' AND '$(TargetFramework)' == 'net47'">$(MonoBasePath)/4.7-api</FrameworkPathOverride>
-    <FrameworkPathOverride Condition="'$(MonoBasePath)' != '' AND '$(TargetFramework)' == 'net471'">$(MonoBasePath)/4.7.1-api</FrameworkPathOverride>
-    <EnableFrameworkPathOverride Condition="'$(MonoBasePath)' != ''">true</EnableFrameworkPathOverride>
-
-    <!-- Add the Facades directory.  Not sure how else to do this. Necessary at least for .NET 4.5 -->
-    <AssemblySearchPaths Condition="'$(MonoBasePath)' != ''">$(FrameworkPathOverride)/Facades;$(AssemblySearchPaths)</AssemblySearchPaths>
-  </PropertyGroup>
-</Project> 
diff --git a/csharp/sbe-dll/ThrowHelper.cs b/csharp/sbe-dll/ThrowHelper.cs
new file mode 100644
index 0000000000..30ca008c9c
--- /dev/null
+++ b/csharp/sbe-dll/ThrowHelper.cs
@@ -0,0 +1,47 @@
+using System;
+
+namespace Org.SbeTool.Sbe.Dll
+{
+    /// <summary>
+    /// Helper class that provides non-returning methods that throw common exception
+    /// from the generated C# code
+    /// </summary>
+    public class ThrowHelper
+    {
+        /// <summary>
+        /// Throws a <see cref="ArgumentOutOfRangeException"/> when the "count" parameter is out of range
+        /// </summary>
+        /// <param name="count">the parameter that triggered the exception</param>
+        public static void ThrowCountOutOfRangeException(int count) =>
+            throw new ArgumentOutOfRangeException("count", $"Outside allowed range: count={count}");
+        
+        /// <summary>
+        /// Throws a <see cref="InvalidOperationException"/> when an invalid operation is invoked on a message (for example: enumerating a group past it's maximal count)
+        /// </summary>
+        public static void ThrowInvalidOperationException() =>
+            throw new InvalidOperationException();
+        
+        /// <summary>
+        /// Throws a <see cref="IndexOutOfRangeException" /> when the "index" parameter is out of range
+        /// </summary>
+        /// <param name="index">the parameter that triggered the exception</param>
+        public static void ThrowIndexOutOfRangeException(int index) =>
+            throw new IndexOutOfRangeException($"index out of range: index={index}");
+        
+        /// <summary>
+        /// Throws a <see cref="ArgumentOutOfRangeException"/> when a too-small <see cref="Span{T}"/>
+        /// is provided to a getter
+        /// </summary>
+        /// <param name="length">The length of the too-small Span</param>
+        public static void ThrowWhenSpanLengthTooSmall(int length) =>
+            throw new ArgumentOutOfRangeException("dst", $"dst.Length={length} is too small.");
+        
+        /// <summary>
+        /// Throws a <see cref="ArgumentOutOfRangeException"/> when a too-large <see cref="Span{T}"/>
+        /// is provided to a setter
+        /// </summary>
+        /// <param name="length">The length of the too-large Span</param>
+        public static void ThrowWhenSpanLengthTooLarge(int length) =>
+            throw new ArgumentOutOfRangeException("src", $"src.Length={length} is too large.");
+    }
+}
\ No newline at end of file
diff --git a/csharp/sbe-dll/sbe-dll.csproj b/csharp/sbe-dll/sbe-dll.csproj
index a556ea09fd..252b6d9f3f 100644
--- a/csharp/sbe-dll/sbe-dll.csproj
+++ b/csharp/sbe-dll/sbe-dll.csproj
@@ -1,6 +1,4 @@
 <Project Sdk="Microsoft.NET.Sdk">
-  <Import Project="..\netfx.props" />
-
   <PropertyGroup>
     <TargetFrameworks>net45;netstandard2.0</TargetFrameworks>
     <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
@@ -23,7 +21,8 @@
   </ItemGroup>
 
   <ItemGroup>
-    <PackageReference Include="System.Memory" Version="4.5.1" />
+    <PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.0-preview.2" PrivateAssets="All" />
+    <PackageReference Include="System.Memory" Version="4.5.3" />
   </ItemGroup>
   
 </Project>
diff --git a/csharp/sbe-tests/sbe-tests.csproj b/csharp/sbe-tests/sbe-tests.csproj
index 53eaca7da5..caf0e97279 100644
--- a/csharp/sbe-tests/sbe-tests.csproj
+++ b/csharp/sbe-tests/sbe-tests.csproj
@@ -1,6 +1,4 @@
 <Project Sdk="Microsoft.NET.Sdk">
-  <Import Project="..\netfx.props" />
-
   <PropertyGroup>
     <TargetFrameworks>net45;netcoreapp2.1</TargetFrameworks>
     <RootNamespace>Org.SbeTool.Sbe.Tests</RootNamespace>
@@ -25,9 +23,10 @@
   </ItemGroup>
 
   <ItemGroup>
-    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.6.1" />
-    <PackageReference Include="MSTest.TestAdapter" Version="1.2.0" />
-    <PackageReference Include="MSTest.TestFramework" Version="1.2.0" />
+    <PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.0-preview.2" PrivateAssets="All" />
+    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.2.0" />
+    <PackageReference Include="MSTest.TestAdapter" Version="1.4.0" />
+    <PackageReference Include="MSTest.TestFramework" Version="1.4.0" />
   </ItemGroup>
 
   <ItemGroup>
diff --git a/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/csharp/CSharpGenerator.java b/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/csharp/CSharpGenerator.java
index 9107c2c5fd..303474b9d7 100644
--- a/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/csharp/CSharpGenerator.java
+++ b/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/csharp/CSharpGenerator.java
@@ -210,14 +210,16 @@ private void generateGroupClassHeader(
         final String typeForBlockLength = cSharpTypeName(tokens.get(index + 2).encoding().primitiveType());
         final String typeForNumInGroup = cSharpTypeName(numInGroupToken.encoding().primitiveType());
 
+        final String throwCondition = numInGroupToken.encoding().applicableMinValue().longValue() == 0 ?
+            "if ((uint) count > %3$d)\n" :
+            "if (count < %2$d || count > %3$d)\n";
+
         sb.append(String.format("\n" +
             indent + INDENT + "public void WrapForEncode(%1$s parentMessage, DirectBuffer buffer, int count)\n" +
             indent + INDENT + "{\n" +
-            indent + INDENT + INDENT + "if (count < %2$d || count > %3$d)\n" +
+            indent + INDENT + INDENT + throwCondition +
             indent + INDENT + INDENT + "{\n" +
-            indent + INDENT + INDENT + INDENT + "throw new ArgumentOutOfRangeException(\"count\",\n" +
-            indent + INDENT + INDENT + INDENT + INDENT + "\"Outside allowed range: count=\" + count +\n" +
-            indent + INDENT + INDENT + INDENT + INDENT + "\", min=%2$d, max=%3$d\");\n" +
+            indent + INDENT + INDENT + INDENT + "ThrowHelper.ThrowCountOutOfRangeException(count);\n" +
             indent + INDENT + INDENT + "}\n\n" +
             indent + INDENT + INDENT + "_parentMessage = parentMessage;\n" +
             indent + INDENT + INDENT + "_buffer = buffer;\n" +
@@ -258,7 +260,7 @@ private void generateGroupEnumerator(final StringBuilder sb, final String groupN
             indent + INDENT + "{\n" +
             indent + INDENT + INDENT + "if (_index + 1 >= _count)\n" +
             indent + INDENT + INDENT + "{\n" +
-            indent + INDENT + INDENT + INDENT + "throw new InvalidOperationException();\n" +
+            indent + INDENT + INDENT + INDENT + "ThrowHelper.ThrowInvalidOperationException();\n" +
             indent + INDENT + INDENT + "}\n\n" +
             indent + INDENT + INDENT + "_offset = _parentMessage.Limit;\n" +
             indent + INDENT + INDENT + "_parentMessage.Limit = _offset + _blockLength;\n" +
@@ -811,9 +813,9 @@ private CharSequence generateArrayProperty(
         sb.append(String.format("\n" +
             indent + "public %1$s Get%2$s(int index)\n" +
             indent + "{\n" +
-            indent + INDENT + "if (index < 0 || index >= %3$d)\n" +
+            indent + INDENT + "if ((uint) index >= %3$d)\n" +
             indent + INDENT + "{\n" +
-            indent + INDENT + INDENT + "throw new IndexOutOfRangeException(\"index out of range: index=\" + index);\n" +
+            indent + INDENT + INDENT + "ThrowHelper.ThrowIndexOutOfRangeException(index);\n" +
             indent + INDENT + "}\n\n" +
             "%4$s" +
             indent + INDENT + "return _buffer.%5$sGet%8$s(_offset + %6$d + (index * %7$d));\n" +
@@ -825,9 +827,9 @@ private CharSequence generateArrayProperty(
         sb.append(String.format("\n" +
             indent + "public void Set%1$s(int index, %2$s value)\n" +
             indent + "{\n" +
-            indent + INDENT + "if (index < 0 || index >= %3$d)\n" +
+            indent + INDENT + "if ((uint) index >= %3$d)\n" +
             indent + INDENT + "{\n" +
-            indent + INDENT + INDENT + "throw new IndexOutOfRangeException(\"index out of range: index=\" + index);\n" +
+            indent + INDENT + INDENT + "ThrowHelper.ThrowIndexOutOfRangeException(index);\n" +
             indent + INDENT + "}\n\n" +
             indent + INDENT + "_buffer.%4$sPut%7$s(_offset + %5$d + (index * %6$d), value);\n" +
             indent + "}\n",
@@ -852,8 +854,7 @@ private CharSequence generateArrayProperty(
                 indent + INDENT + "const int length = %2$d;\n" +
                 indent + INDENT + "if (dst.Length < length)\n" +
                 indent + INDENT + "{\n" +
-                indent + INDENT + INDENT +
-                "throw new ArgumentOutOfRangeException($\"dst.Length={dst.Length} is too large.\");\n" +
+                indent + INDENT + INDENT + "ThrowHelper.ThrowWhenSpanLengthTooSmall(dst.Length);\n" +
                 indent + INDENT + "}\n\n" +
                 "%3$s" +
                 indent + INDENT + "_buffer.GetBytes(_offset + %4$d, dst);\n" +
@@ -874,8 +875,7 @@ private CharSequence generateArrayProperty(
                 indent + INDENT + "const int length = %2$d;\n" +
                 indent + INDENT + "if (src.Length > length)\n" +
                 indent + INDENT + "{\n" +
-                indent + INDENT + INDENT +
-                "throw new ArgumentOutOfRangeException($\"src.Length={src.Length} is too large.\");\n" +
+                indent + INDENT + INDENT + "ThrowHelper.ThrowWhenSpanLengthTooLarge(src.Length);\n" +
                 indent + INDENT + "}\n\n" +
                 indent + INDENT + "_buffer.SetBytes(_offset + %3$d, src);\n" +
                 indent + "}\n",