Mark some structs as byreflike #104870
Mark some structs as byreflike #104870
Conversation
dotnet-issue-labeler
bot
added
the
needs-area-label
| @@ -654,7 +654,7 @@ internal unsafe bool WriteEvent(ref EventDescriptor eventDescriptor, IntPtr even | |||
| } | |||
|
|
|||
| /// <summary>Workaround for inability to stackalloc object[EtwAPIMaxRefObjCount == 8].</summary> | |||
| private struct EightObjects | |||
| private ref struct EightObjects | |||
| { | |||
| internal object? _arg0; | |||
There was a problem hiding this comment.
This should be InlineArray. Can InlineArrays be ref structs?
There was a problem hiding this comment.
It would result in a warning:
warning CS9184: 'Inline arrays' language feature is not supported for an inline array type that is not valid as a type argument, or has element type that is not valid as a type argument.
IMO this diagnostic message is out-of-date as in C# 13 we have added the support for byref-like type arguments. /cc: @jaredpar
There was a problem hiding this comment.
@AaronRobinsonMSFT is this supported in the runtime?
There was a problem hiding this comment.
This should be InlineArray. Can InlineArrays be
ref structs?
Yes.
@AaronRobinsonMSFT is this supported in the runtime?
As in, can ref structs be marked as InlineArray or something else? If the former, see above.
There was a problem hiding this comment.
Applied InlineArray with #pragma warning disable CS9184
There was a problem hiding this comment.
Is this one of the cases though where we have explicit guidance that Unsafe.As is safe to use in the compiler? Basically is @jkotas signing off that use extending Unsafe.As where the source is a ref struct in the InlineArray case is supported?
There was a problem hiding this comment.
extending Unsafe.As where the source is a ref struct in the InlineArray case is supported
Yes, that's fine. It does not fundamentally change the special sequence to create Span from InlineArray that has to work forever.
(I would have preferred if that special sequence did not use Unsafe.As - dotnet/csharplang#7064 (comment) , but that ship sailed.)
There was a problem hiding this comment.
Is that a place where we could define a new runtime helper method and adjust compiler to use that going forward? Won't fix the existing debt but going forward if there was a better helper to use we could move to it.
There was a problem hiding this comment.
Something like:
public class RuntimeHelpers
{
public static Span<Element> InlineArrayAsSpan<InlineArray, Element>(ref InlineArray inlineArray) where InlineArray: allows ref struct, Element: allows ref struct;
}? I agree that it looks better than the current solution with Unsafe.As. It is definitely possible; it is a bunch of work to teach all codegens to expand it as intrinsic or throw exception if there is any sort of mismatch.
...raries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEBinaryReader.cs
Outdated
Show resolved
Hide resolved
|
@jkotas can we merge as is then? I couldn't find many good candidates with codegen improvements (I wrote an adhoc roslyn analyzer to find them), diffs: MihuBot/runtime-utils#525 removed a few write barriers |
| @@ -2258,7 +2258,7 @@ public static bool TrueForAll<T>(T[] array, Predicate<T> match) | |||
| 0X7FFFFFC7; | |||
|
|
|||
| // Private value type used by the Sort methods. | |||
| private readonly struct SorterObjectArray | |||
| private readonly ref struct SorterObjectArray | |||
There was a problem hiding this comment.
Is it worth adding a comment about why ref exists, here and elsewhere? e.g.
| private readonly ref struct SorterObjectArray | |
| private readonly ref struct SorterObjectArray // ref isn't functionally necessary but enables additional JIT optimization |
It's a little unfortunate we're going to start to see this sprinkled around. Is there anything more we could do in the JIT to effectively infer it?
There was a problem hiding this comment.
Yeah a comment makes sense.
It's a little unfortunate we're going to start to see this sprinkled around.
I don't see a difference with other things we sprinkle around like marking classes as sealed, etc 🤷
Is there anything more we could do in the JIT to effectively infer it?
Only NativeAOT might I guess..
There was a problem hiding this comment.
I don't see a difference with other things we sprinkle around like marking classes as sealed, etc 🤷
Sealed says "this type wasn't designed to be derived from, please think really hard before you remove the sealed as usage might not expect it"; it has use separate from the perf angle, in fact most of the perf angle came long after it was introduced. Making these types ref doesn't IMO have any such design benefit; it's being done solely to get a JIT optimization to kick in. You typically only make a type ref out of necessity because it needs to contain something ref.
There was a problem hiding this comment.
Ok, you're right - I don't have a strong opinion on this, I was just testing my script and hoped to see some nice codegen diffs, but they're quite small
Just a CI codegen test