Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C#] Use throw helpers for generated code #588

Closed
damageboy opened this issue Sep 30, 2018 · 4 comments · Fixed by #695
Closed

[C#] Use throw helpers for generated code #588

damageboy opened this issue Sep 30, 2018 · 4 comments · Fixed by #695

Comments

@damageboy
Copy link
Contributor

damageboy commented Sep 30, 2018

Currently the generated code in the C# generated Getters/Setters for fixed array types and a few other cases calls a rather lengthy set of operations for the uncommon path of throwing an exception in case the provided parameters were not constructed properly.

While this is of course important, for the general case, this is also somewhat inefficient in its current form as the code to generate the exception is being repeated many times, leading to generated code bloat, esp. when it comes down to the JITed code size.

I'm copy-pasting this from CoreCLR's own ThrowHelper.cs:

// This file defines an internal class used to throw exceptions in BCL code.
// The main purpose is to reduce code size. 
// 
// The old way to throw an exception generates quite a lot IL code and assembly code.
// Following is an example:
//     C# source
//          throw new ArgumentNullException(nameof(key), SR.ArgumentNull_Key);
//     IL code:
//          IL_0003:  ldstr      "key"
//          IL_0008:  ldstr      "ArgumentNull_Key"
//          IL_000d:  call       string System.Environment::GetResourceString(string)
//          IL_0012:  newobj     instance void System.ArgumentNullException::.ctor(string,string)
//          IL_0017:  throw
//    which is 21bytes in IL.
// 
// So we want to get rid of the ldstr and call to Environment.GetResource in IL.
// In order to do that, I created two enums: ExceptionResource, ExceptionArgument to represent the
// argument name and resource name in a small integer. The source code will be changed to 
//    ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key, ExceptionResource.ArgumentNull_Key);
//
// The IL code will be 7 bytes.
//    IL_0008:  ldc.i4.4
//    IL_0009:  ldc.i4.4
//    IL_000a:  call       void System.ThrowHelper::ThrowArgumentNullException(valuetype System.ExceptionArgument)
//    IL_000f:  ldarg.0
//
// This will also reduce the Jitted code size a lot. 
//
// It is very important we do this for generic classes because we can easily generate the same code 
// multiple times for different instantiation. 
// 

This is even more severe in the generated SBE code, as due to string formatting each such block is 27 bytes long rather than the 21 mentioned in CoreCLR for their "pattern".

I would like to work on adding a minimal ThrowHelper.cs to SBE.dll and do the work of calling the throw helpers rather than generated the current exception throwing code, Any thoughts or comments about this?

@damageboy
Copy link
Contributor Author

Any comment?

@ZachBray
Copy link
Contributor

ZachBray commented Oct 9, 2018

Seems like a good idea to me. We can fit more "useful" instructions in the cache.

@damageboy
Copy link
Contributor Author

@ZachBray Yes, another side-effect of this is that this potentially makes some/all getters inlineable by the JIT by default without needing to force inlining with things such as AggressiveInlining.

@billsegall
Copy link
Contributor

billsegall commented Oct 11, 2018 via email

damageboy added a commit to damageboy/simple-binary-encoding that referenced this issue Jul 16, 2019
- 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: dotnet/coreclr#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 real-logic#588
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants