Faster Guard APIs #7
Conversation
|
So what's missing here? |
|
@JimBobSquarePants The PR itself is done for now, but @antonfirsov mentioned he wanted me to use T4 to add more type-specific implementations of the APIs. I could do that now if you wont, otherwise we can just merge this as is and then add those later on. Up to you 👍 |
|
@Sergio0694 A single PR would be easier for us to update across all the repositories. If you could do the T4 changes that would be great. Thanks! |
|
@JimBobSquarePants I've added the T4 template, and I've tested it with a local .NET Core template to make sure it works. I can't get it to generate the Feel free to take a look if you have an idea on how to get the T4 generator to work here! 😊 |
|
@Sergio0694 I'll have a look for you tomorrow. Thanks for the update! |
|
No problem, happy to help! 😄 |
|
@Sergio0694 I fixed the T4 templates and did some other cleanup. There's a couple of failing Guard tests though that need to fixed though. I couldn't do them as I didn't know what the intended message was. To build the template you need to use the Build -> Transform All T4 Templates command. |
| public const MethodImplOptions ShortMethod = MethodImplOptions.NoInlining; | ||
| #else | ||
| #if SUPPORTS_HOTPATH | ||
| public const MethodImplOptions HotPath = MethodImplOptions.AggressiveOptimization; |
There was a problem hiding this comment.
@JimBobSquarePants Possibly dumb quesiton, why are we just using MethodImplOptions.AggressiveOptimization in this case instead of MethodImplOptions.AggressiveInlining | MethodImplOptions.AggressiveOptimization? I've seen them used at the same time quote often around CoreCLR, and I don't think that the optimization flag implies inlining as well - don't we want both those JIT perks for hot paths? 🤔
There was a problem hiding this comment.
I've only seen them used as flags
MethodImplOptions.AggressiveInlining | MethodImplOptions.AggressiveOptimization
I haven't managed to find any documentation on the new attribute at all, nor could I find the issue that introduced it so I thought I better keep them separate as the original enum defines.
There was a problem hiding this comment.
Maybe we should combine them. @antonfirsov Do you have any insight?
There was a problem hiding this comment.
There was a problem hiding this comment.
@Sergio0694 Looks like you definitely want to keep them separate. Relevant conversation starts here.
There was a problem hiding this comment.
@JimBobSquarePants I'm not sure what you mean there, what exception message? I mean, I can see the throw methods for the out of range errors with a formatted message already, what are you referring to here?
There was a problem hiding this comment.
@Sergio0694 I suppose inlining is not the only magic provided by AggressiveOptimization, but you're right Guard methods are so short, there really isn't anything to agressively optimize, and if you say it might bring other risks, let's leave it out for now!
There was a problem hiding this comment.
I'm not 100% sure since that conversation was really long and I could just read up to some point, but apparently there were a lot of various implications depending on the scenario. The main takeaway I got was this:
• AggressiveInlining just pushes the JIT to inline the (marked) callee into its caller
• AggressiveOptimization pushes the JIT to try to inline callees from their (marked) caller (the one with the attribute). It also suggests to go straight to the best JIT tiering possible.
In out case, the Guard APIs don't call functions that could benefit from inlinig, in fact all the helpers should not be inlined, and are marked as such. We only care about those Guard APIs themselves being inlined into their callers, so yeah I'd say (at least for now) the safe bet would be to just use AggressiveInlining 👍
There was a problem hiding this comment.
@Sergio0694 What I mean is that the unit tests expect a different value for the message than the actual output. Shall I just update the test to match?
There was a problem hiding this comment.
Ah my bad, I didn't consider the fact the tests were also matching the exact error message as well. Sure thing, yeah that would be perfect then 👍
|
So did you figure out how to make VS generate the |
|
If you push the cs then any time you ask VS to rebuild templates it alters the cs file line endings and causes the submodule to be marked dirty. You then have to reset the submodule which is a chore. I figured out a hack whereby we deliberately ignore the generated file in the submodule but use a pre build target to copy the file into the solution so that we have a tracked copy. I'm looking at an xplat solution that will allow the consuming library to convert all T4 templates on build and save the copy. |
As discussed with @antonfirsov, this PR restructures the
GuardAPIs to reduce the inlined asm code. Goes from this to this.