Avoid some defensive copies on readonly structs in System.Net.Quic #101133

rzikm · 2024-04-16T18:26:38Z

This removes defensive copies on readonly structs in S.N.S uncovered by an analyzer from a community member (thanks @hamarb123!).

dotnet-policy-service · 2024-04-16T18:27:03Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

MihaZupan · 2024-04-16T18:35:16Z

cc: @hamarb123

MihaZupan

Thanks

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs

rzikm · 2024-04-16T19:21:02Z

/azp run runtime

azure-pipelines · 2024-04-16T19:21:30Z

Azure Pipelines successfully started running 1 pipeline(s).

src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/msquic.cs

ManickaP · 2024-04-17T08:26:10Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/msquic.cs

@@ -20,7 +20,7 @@ namespace Microsoft.Quic
 {
    internal unsafe partial struct QUIC_BUFFER
    {
-        public Span<byte> Span => new(Buffer, (int)Length);
+        public readonly Span<byte> Span => new(Buffer, (int)Length);


I don't remember exactly, but if this is generated code from MsQuic, it should be fixed there as well.

right, I thought this was added manually by us (since the QUIC_BUFFER definition is in another auto-generated file). I will follow up to make the change in their repo as well.

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs

…uicConfiguration.Cache.cs Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs

rzikm · 2024-04-22T11:47:54Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2024-04-22T11:48:03Z

Azure Pipelines successfully started running 1 pipeline(s).

src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicInteropTests.cs

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ReceiveBuffers.cs

stephentoub · 2024-04-22T13:52:37Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/QUIC_SETTINGS.IEquattable.cs

+
+namespace Microsoft.Quic
+{
+    internal partial struct QUIC_SETTINGS : System.IEquatable<QUIC_SETTINGS>


Why is this needed? Is QUIC_SETTINGS being put into a Dictionary somewhere or something?

QUIC_SETTINGS is used as part of the cache key for MsQuicConfigurationHandle

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs

Line 101 in e12e2fa

Settings.Equals(other.Settings) &&

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs

ManickaP · 2024-04-22T13:57:20Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs

@@ -88,7 +88,7 @@ internal sealed class MsQuicContextSafeHandle : MsQuicSafeHandle
    /// Holds a weak reference to the managed instance.
    /// It allows delegating MsQuic events to the managed object while it still can be collected and finalized.
    /// </summary>
-    private readonly GCHandle _context;
+    private GCHandle _context;


Unintended leftover change?

This one should stay, otherwise the .Free() is executed on a defensive copy

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs

Line 133 in 1c7b419

_context.Free();

It probably is not a big deal since ReleaseHandle runs exactly once, but we still make sure to cleanup dangling pointers.

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs

ManickaP

Nits, otherwise LGTM.

src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/QUIC_SETTINGS.IEquattable.cs

CarnaViire

LGTM

rzikm · 2024-04-25T16:32:15Z

CI failures are unrelated.

…otnet#101133) * Avoid some defensive copies on readonly structs in System.Net.Quic * Keep readonly-ness of CacheKey * Apply suggestions from code review * Remove ReadOnlySpan property * Update src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com> * Code review feedback * Implement IEquattable for QUIC_SETTINGS * More code review feedback * Code review feedback --------- Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>

Avoid some defensive copies on readonly structs in System.Net.Quic

1c7b419

dotnet-issue-labeler bot added the area-System.Net label Apr 16, 2024

dotnet-policy-service bot assigned rzikm Apr 16, 2024

rzikm requested a review from a team April 16, 2024 18:28

liveans approved these changes Apr 16, 2024

View reviewed changes

Keep readonly-ness of CacheKey

4398150

MihaZupan approved these changes Apr 16, 2024

View reviewed changes

hamarb123 approved these changes Apr 16, 2024

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs Outdated Show resolved Hide resolved

Apply suggestions from code review

45bbf45

hamarb123 reviewed Apr 16, 2024

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/msquic.cs Outdated Show resolved Hide resolved

Remove ReadOnlySpan property

8f5ae6d

ManickaP reviewed Apr 17, 2024

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs Outdated Show resolved Hide resolved

rzikm and others added 2 commits April 17, 2024 11:33

Update src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQ…

c69ba96

…uicConfiguration.Cache.cs Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>

Code review feedback

7821590

CarnaViire reviewed Apr 17, 2024

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Apr 17, 2024

Timeout in System.Text.Json.Tests.Utf8JsonWriterTests.WriteNumbers #101193

Closed

Implement IEquattable for QUIC_SETTINGS

1df80d9

CarnaViire reviewed Apr 22, 2024

View reviewed changes

src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicInteropTests.cs Show resolved Hide resolved

stephentoub reviewed Apr 22, 2024

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ReceiveBuffers.cs Outdated Show resolved Hide resolved

stephentoub reviewed Apr 22, 2024

View reviewed changes

ManickaP reviewed Apr 22, 2024

View reviewed changes

More code review feedback

2440bf0

build-analysis bot mentioned this pull request Apr 22, 2024

Timeout in System.Net.Quic.Functional.Tests #86019

Closed

ManickaP approved these changes Apr 25, 2024

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/QUIC_SETTINGS.IEquattable.cs Outdated Show resolved Hide resolved

src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/QUIC_SETTINGS.IEquattable.cs Outdated Show resolved Hide resolved

Code review feedback

b994c47

CarnaViire approved these changes Apr 25, 2024

View reviewed changes

rzikm merged commit aa07770 into dotnet:main Apr 25, 2024
78 of 84 checks passed

build-analysis bot mentioned this pull request Apr 25, 2024

[CI] Test failure from System.Net.Http.Functional.Tests.SocketsHttpHandlerTest_Cookies_Http3.GetAsync_DefaultCoookieContainer_NoCookieSent #101567

Closed

karelz added this to the 9.0.0 milestone May 14, 2024

github-actions bot locked and limited conversation to collaborators Jun 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid some defensive copies on readonly structs in System.Net.Quic #101133

Avoid some defensive copies on readonly structs in System.Net.Quic #101133

rzikm commented Apr 16, 2024 •

edited

Loading

dotnet-policy-service bot commented Apr 16, 2024

MihaZupan commented Apr 16, 2024

MihaZupan left a comment

rzikm commented Apr 16, 2024

azure-pipelines bot commented Apr 16, 2024

ManickaP Apr 17, 2024

rzikm Apr 17, 2024

rzikm commented Apr 22, 2024

azure-pipelines bot commented Apr 22, 2024

stephentoub Apr 22, 2024

rzikm Apr 22, 2024

ManickaP Apr 22, 2024

rzikm Apr 22, 2024

ManickaP left a comment

CarnaViire left a comment

rzikm commented Apr 25, 2024

Avoid some defensive copies on readonly structs in System.Net.Quic #101133

Avoid some defensive copies on readonly structs in System.Net.Quic #101133

Conversation

rzikm commented Apr 16, 2024 • edited Loading

dotnet-policy-service bot commented Apr 16, 2024

MihaZupan commented Apr 16, 2024

MihaZupan left a comment

Choose a reason for hiding this comment

rzikm commented Apr 16, 2024

azure-pipelines bot commented Apr 16, 2024

ManickaP Apr 17, 2024

Choose a reason for hiding this comment

rzikm Apr 17, 2024

Choose a reason for hiding this comment

rzikm commented Apr 22, 2024

azure-pipelines bot commented Apr 22, 2024

stephentoub Apr 22, 2024

Choose a reason for hiding this comment

rzikm Apr 22, 2024

Choose a reason for hiding this comment

ManickaP Apr 22, 2024

Choose a reason for hiding this comment

rzikm Apr 22, 2024

Choose a reason for hiding this comment

ManickaP left a comment

Choose a reason for hiding this comment

CarnaViire left a comment

Choose a reason for hiding this comment

rzikm commented Apr 25, 2024

rzikm commented Apr 16, 2024 •

edited

Loading