Mark readonly structs as such #311

benaadams · 2018-02-04T20:15:56Z

Means they can be used efficiently with in

Resolves: dotnet/aspnetcore#2856
Resolves: dotnet/aspnetcore#2857
Resolves: dotnet/aspnetcore#2858

davidfowl · 2018-02-04T20:29:47Z

src/Microsoft.Extensions.Primitives/Microsoft.Extensions.Primitives.csproj

@@ -12,6 +12,10 @@ Microsoft.Extensions.Primitives.StringSegment</Description>
    <PackageTags>primitives</PackageTags>
  </PropertyGroup>

+  <PropertyGroup>


We should do this globally /cc @natemcmaster @Eilon

This has come up a few times lately and the answer I got from @natemcmaster is that we haven't done this because VSCode doesn't have it yet.

It also looks like Omnisharp (the library) added it 5 days ago https://github.com/OmniSharp/omnisharp-roslyn/releases but Omnisharp (the VS Code extension) hasn't yet https://github.com/OmniSharp/omnisharp-vscode/releases

@DustinCampbell release a new omnisharp 😄

We'll make it our default once we have better tooling support. cref aspnet/BuildTools#566

It's in now.

davidfowl · 2018-02-04T20:30:15Z

This isn't a breaking change right?

benaadams · 2018-02-04T20:32:13Z

This isn't a breaking change right?

Don't think so DateTime and KeyValuePair<TKey, TValue> have been changed in the framework to readonly... dotnet/coreclr#14789

benaadams · 2018-02-04T20:34:26Z

Yes, not breaking dotnet/coreclr#14789 (comment)

weshaggard on Jan 5 Member
I'm working on the APICompat checks work but I'm not sure if this a compatible change. jkotas stephentoub is this an API compatible change?

jkotas on Jan 5 Member
Marking struct as readonly should be compatible change. The other direction (removing readonly on a struct) is not a compatible change.

jaredpar on Jan 5 Member
Yes this is a compatible change.

davidfowl · 2018-02-04T20:37:55Z

ref -> in?

benaadams · 2018-02-04T20:43:51Z

ref -> in?

Enumerator .ctors should have been internal 😄

@jkotas @jaredpar is changing ref -> in a binary breaking change? Or just a C# compile time change? Or neither

benaadams · 2018-02-04T20:47:58Z

Can revert back to ref

benaadams · 2018-02-04T20:50:13Z

Except can't pass this by ref 😢

benaadams · 2018-02-04T20:57:34Z

Can't define a second internal in .ctor as they conflict

internal Enumerator(in StringTokenizer tokenizer)
public Enumerator(ref StringTokenizer tokenizer)

rynowak · 2018-02-04T21:08:41Z

src/Microsoft.Extensions.Primitives/StringTokenizer.cs

@@ -57,7 +57,7 @@ public struct Enumerator : IEnumerator<StringSegment>
            private readonly char[] _separators;
            private int _index;

-            public Enumerator(ref StringTokenizer tokenizer)
+            public Enumerator(in StringTokenizer tokenizer)


Is this a breaking change?

davidfowl · 2018-02-04T21:10:09Z

Yea I think this isn't doable without breaking changes. We should make sure it's detected as such with api check.

benaadams · 2018-02-04T21:12:50Z

Think can do a no-breaking change variant

benaadams · 2018-02-04T21:20:00Z

Should be ok now?

benaadams · 2018-02-04T21:30:17Z

Will also mean the enumerator for StringTokenizer calling methods on the readonly field StringSegment _value in MoveNext will suck less :)

As it will no longer cause a copy on each iteration and a GC write barrier with it

benaadams · 2018-02-04T22:15:09Z

And update the methods to take advantage 😄

benaadams · 2018-02-04T22:36:11Z

Your breaking change tool picked up the adding of in being breaking, although its not?

tannergooding · 2018-02-05T00:05:55Z

src/Microsoft.Extensions.Primitives/InplaceStringBuilder.cs

@@ -47,7 +47,7 @@ public unsafe void Append(string s)
            Append(s, 0, s.Length);
        }

-        public void Append(StringSegment segment)
+        public void Append(in StringSegment segment)


This is breaking, it changes the signature to be the equivalent of ref, but with an additional readonly attribute for compiler disambiguation (the ref changes the signature, the attribute does not)

If an additional method was added, which would the compiler prefer, as to the caller they are the same sig?

public void Append(StringSegment segment) public void Append(in StringSegment segment)

Seems it gets confused

Error CS0121 The call is ambiguous between the following methods or properties:
'StringSegment.Equals(StringSegment)' and 'StringSegment.Equals(in StringSegment)'

The "confusion" is getting resolved in 7.3, iirc.

The byval will always win by default and you will need to explicitly specify in to call the other overload.

If you can call the in without in when there is only one overload; then it should clearly also win when its a readonly struct as there is no disadvantage to it winning only an advantage?

That depends on the size of the struct, the underlying ABI, and whether or not you want different overload resolution for a normal struct vs a readonly struct

Differing read only vs normal can see as perhaps weird.

As to size and ABI; overload is going to be the same regardless; so really that's a question of whether the overload should have been written in the first place; since it has there must be a reason (unless you are ifdefing it)

And if you are ifdefing it, you just ifdef out the in overload if that's not suitable for the platform

I was primarily indicating there may be some scenarios where it isn't always advantageous.

As to size and ABI; overload is going to be the same regardless; so really that's a question of whether the overload should have been written in the first place; since it has there must be a reason (unless you are ifdefing it)

Yes, and defaulting to the byval overload is, IMO, the "better" default (from a language perspective). It means that we don't need another keyword to indicate the byval overload should be called, it means existing code will continue calling the same overload they did before, and that they can explicitly opt-in to calling the new overload if needed/where desired.

benaadams · 2018-02-05T02:06:16Z

Should be also binary compat now

tannergooding · 2018-02-05T02:57:50Z

src/Microsoft.Extensions.Primitives/Extensions.cs

+        /// <param name="builder">The <see cref="StringBuilder"/> to add to.</param>
+        /// <param name="segment">The <see cref="StringSegment"/> to add.</param>
+        /// <returns>The original <see cref="StringBuilder"/>.</returns>
+        public static StringBuilder Append(this StringBuilder builder, in StringSegment segment)


I believe this is a source breaking change until 7.3.

Anyone that was previously calling the public static StringBuilder Append(this StringBuilder builder, StringSegment segment) overload will now get a CS0121: The call is ambiguous between the following methods or properties (if they are using the 7.2 version of the compiler).

There is also some weirdness in that the v7.1 (or prior) version of the compiler will allow the above, as it will see the new overload as ref and allow the compiler to call it and disambiguate with ref, but that will break if recompiled using v7.2+ since ref cannot be used for in parameters (I'm going to log a bug for this in a minute).

dotnet/csharplang#1308

benaadams · 2018-02-05T04:53:25Z

Ok, hopefully its all happily compat now :)

benaadams · 2018-02-05T19:33:24Z

Rebased

benaadams · 2018-02-05T19:34:43Z

src/Microsoft.Extensions.Primitives/StringTokenizer.cs

@@ -31,21 +43,21 @@ public StringTokenizer(string value, char[] separators) : this((StringSegment)va
        /// <param name="separators">The characters to tokenize by.</param>
        public StringTokenizer(StringSegment value, char[] separators)
        {
-            if (value == null)
+            if (!value.HasValue)


Currently converts null into a new StringValues(null) then does ==(StringValues, StringValues) on it

halter73

Though this is the first time noticing ExceptionArgument. Not a fan.

benaadams · 2018-02-17T07:04:48Z

Though this is the first time noticing ExceptionArgument. Not a fan.

Mainly for inlining and code size; and inlined code size repetition, also particularly with generics; this the lightest touch other than combining or passing no arguments; as its just a number:

ThrowHelper.ThrowArgumentNullException(ExceptionArgument.value);
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.separators);

is

G_M1150_IG06:
       B907000000           mov      ecx, 7
       E8C8FCFFFF           call     ThrowHelper:ThrowArgumentNullException(int)

G_M1150_IG07:
       B908000000           mov      ecx, 8
       E8BEFCFFFF           call     ThrowHelper:ThrowArgumentNullException(int)
       CC                   int3

Passing a string with nameof increases the il to deal with the load and can push it passed inline due to the increased number of steps; though the asm isn't hugely bigger

ThrowHelper.ThrowArgumentNullException(nameof(value));
ThrowHelper.ThrowArgumentNullException(nameof(separators));

is

G_M1150_IG06:
       48B9683025DE40010000 mov      rcx, 0x140DE253068
       488B09               mov      rcx, gword ptr [rcx]
       E8C0FCFFFF           call     ThrowHelper:ThrowArgumentNullException(ref)

G_M1150_IG07:
       48B9703025DE40010000 mov      rcx, 0x140DE253070
       488B09               mov      rcx, gword ptr [rcx]
       E8AEFCFFFF           call     ThrowHelper:ThrowArgumentNullException(ref)
       CC                   int3

Throwing inline however is an entirely different beast:

throw new ArgumentNullException(nameof(value));
throw new ArgumentNullException(nameof(separators));

is

G_M1150_IG06:
       48B9A84C9C89FD7F0000 mov      rcx, 0x7FFD899C4CA8
       E8D71AB05F           call     CORINFO_HELP_NEWSFAST
       488BF0               mov      rsi, rax
       B903000000           mov      ecx, 3
       48BA204DA054FD7F0000 mov      rdx, 0x7FFD54A04D20
       E86055AE5F           call     CORINFO_HELP_STRCNS
       488BD0               mov      rdx, rax
       488BCE               mov      rcx, rsi
       E805CE7834           call     ArgumentNullException:.ctor(ref):this
       488BCE               mov      rcx, rsi
       E81DD8A85F           call     CORINFO_HELP_THROW

G_M1150_IG07:
       48B9A84C9C89FD7F0000 mov      rcx, 0x7FFD899C4CA8
       E89E1AB05F           call     CORINFO_HELP_NEWSFAST
       488BF0               mov      rsi, rax
       B90F000000           mov      ecx, 15
       48BA204DA054FD7F0000 mov      rdx, 0x7FFD54A04D20
       E82755AE5F           call     CORINFO_HELP_STRCNS
       488BD0               mov      rdx, rax
       488BCE               mov      rcx, rsi
       E8CCCD7834           call     ArgumentNullException:.ctor(ref):this
       488BCE               mov      rcx, rsi
       E8E4D7A85F           call     CORINFO_HELP_THROW
       CC                   int3

benaadams · 2018-02-17T07:11:56Z

The Jit won't split functions (except when doing AoT; where it does move the throws out-of-line in the actual assembly); so it would be nice if the C# compiler created ThrowHelpers, but last I asked the feedback was the Jit should do it.

The Jit does move the throws (and ThrowHelper calls) to the end of the method and the ifs are unpredicted; so the main cost is lost inlinining opportunities and a CPU instruction cache that's not dense with hot code but has lots of cold non-executed stuff in it.

But inlining is the uber optimization; as long as its not bringing lots of junk with it...

davidfowl · 2018-02-20T05:41:07Z

@benaadams can you remove the manual C# 7.2 project file, it should be our default now.

benaadams · 2018-02-20T05:49:53Z

Removed

davidfowl · 2018-02-20T05:53:34Z

test/Microsoft.Extensions.Primitives.Tests/Microsoft.Extensions.Primitives.Tests.csproj

-
+
+  <PropertyGroup>
+    <LangVersion>7.2</LangVersion>


@benaadams missed one

benaadams · 2018-02-20T06:14:10Z

Removed :)

benaadams · 2018-02-23T03:26:32Z

Rebased

davidfowl · 2018-02-23T17:31:10Z

@pakrym can you take a look?

pakrym · 2018-02-24T05:21:32Z

src/Microsoft.Extensions.Primitives/StringTokenizer.cs

            }

            _value = value;
            _separators = separators;
        }

-        public Enumerator GetEnumerator() => new Enumerator(ref this);
+        public Enumerator GetEnumerator() => new Enumerator(in _value, _separators);


Does in help codegen in this case? In theory ctor should be inlined and parameter copy eliminated.

pakrym · 2018-02-24T05:22:30Z

I'm fine with this change, I just think that we should start from being conservative with using in and see if it really helps things. It's to easy to start putting it everywhere where we take struct as an argument.

davidfowl · 2018-02-27T23:22:16Z

src/Microsoft.Extensions.Primitives/StringValues.cs

@@ -254,6 +254,44 @@ public static StringValues Concat(StringValues values1, StringValues values2)
            return new StringValues(combined);
        }

+        public static StringValues Concat(in StringValues values, string value)


@benaadams Is this for kestrel?

This reverts commit 38fb78b.

benaadams changed the title ~~Makes readonly structs as such~~ Mark readonly structs as such Feb 4, 2018

davidfowl reviewed Feb 4, 2018

View reviewed changes

rynowak reviewed Feb 4, 2018

View reviewed changes

benaadams force-pushed the readonly-structs branch from b0fbd7f to 23a704c Compare February 4, 2018 22:27

benaadams mentioned this pull request Feb 4, 2018

Pass StringValues via in aspnet/KestrelHttpServer#2295

Merged

benaadams mentioned this pull request Feb 4, 2018

Update for in, readonly struct, ref struct dotnet/corefx#26841

Merged

tannergooding reviewed Feb 5, 2018

View reviewed changes

benaadams force-pushed the readonly-structs branch from 1b1b940 to 5e8bb44 Compare February 5, 2018 01:59

tannergooding reviewed Feb 5, 2018

View reviewed changes

benaadams force-pushed the readonly-structs branch from 29b0afc to 51db527 Compare February 5, 2018 04:44

benaadams force-pushed the readonly-structs branch from 51db527 to 31a0de4 Compare February 5, 2018 19:32

benaadams commented Feb 5, 2018

View reviewed changes

halter73 approved these changes Feb 16, 2018

View reviewed changes

davidfowl reviewed Feb 20, 2018

View reviewed changes

benaadams added 4 commits February 23, 2018 03:26

Mark readonly structs as such

f71a920

Concat tests

ce58795

Remove 7.2

269172c

Remove 7.2

4f24d65

benaadams force-pushed the readonly-structs branch from ca5f681 to 4f24d65 Compare February 23, 2018 03:26

davidfowl approved these changes Feb 23, 2018

View reviewed changes

pakrym approved these changes Feb 24, 2018

View reviewed changes

pakrym reviewed Feb 24, 2018

View reviewed changes

benaadams mentioned this pull request Feb 26, 2018

ReadOnlySequence efficiency+inlines+string dotnet/corefx#27455

Merged

davidfowl reviewed Feb 27, 2018

View reviewed changes

davidfowl approved these changes Feb 27, 2018

View reviewed changes

davidfowl merged commit 38fb78b into dotnet:dev Feb 27, 2018

benaadams deleted the readonly-structs branch February 28, 2018 08:04

pakrym added a commit that referenced this pull request Feb 28, 2018

Revert "Mark readonly structs as such (#311)"

e6eb703

This reverts commit 38fb78b.

dotnet locked as resolved and limited conversation to collaborators May 30, 2023

Mark readonly structs as such #311

Mark readonly structs as such #311

Conversation

benaadams commented Feb 4, 2018 • edited

davidfowl Feb 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Feb 4, 2018

benaadams commented Feb 4, 2018 • edited

benaadams commented Feb 4, 2018

davidfowl commented Feb 4, 2018

benaadams commented Feb 4, 2018 • edited

benaadams commented Feb 4, 2018

benaadams commented Feb 4, 2018

benaadams commented Feb 4, 2018

Choose a reason for hiding this comment

davidfowl commented Feb 4, 2018

benaadams commented Feb 4, 2018

benaadams commented Feb 4, 2018

benaadams commented Feb 4, 2018 • edited

benaadams commented Feb 4, 2018

benaadams commented Feb 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams Feb 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams Feb 5, 2018 • edited

Choose a reason for hiding this comment

benaadams Feb 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams commented Feb 5, 2018

tannergooding Feb 5, 2018 • edited

Choose a reason for hiding this comment

tannergooding Feb 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams commented Feb 5, 2018

benaadams commented Feb 5, 2018

Choose a reason for hiding this comment

halter73 left a comment

Choose a reason for hiding this comment

benaadams commented Feb 17, 2018

benaadams commented Feb 17, 2018 • edited

davidfowl commented Feb 20, 2018

benaadams commented Feb 20, 2018

Choose a reason for hiding this comment

benaadams commented Feb 20, 2018

benaadams commented Feb 23, 2018

davidfowl commented Feb 23, 2018

Choose a reason for hiding this comment

pakrym commented Feb 24, 2018 • edited

Choose a reason for hiding this comment

benaadams commented Feb 4, 2018 •

edited

davidfowl Feb 4, 2018 •

edited

benaadams commented Feb 4, 2018 •

edited

benaadams commented Feb 4, 2018 •

edited

benaadams commented Feb 4, 2018 •

edited

benaadams Feb 5, 2018 •

edited

benaadams Feb 5, 2018 •

edited

benaadams Feb 5, 2018 •

edited

tannergooding Feb 5, 2018 •

edited

tannergooding Feb 5, 2018 •

edited

benaadams commented Feb 17, 2018 •

edited

pakrym commented Feb 24, 2018 •

edited