Avoid some defensive copies on readonly structs in System.Net.Quic by rzikm · Pull Request #101133 · dotnet/runtime

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

    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

    /// 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

ghost 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

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

Conversation

rzikm commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-service bot commented Apr 16, 2024

Uh oh!

MihaZupan commented Apr 16, 2024

Uh oh!

MihaZupan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rzikm commented Apr 16, 2024

Uh oh!

azure-pipelines bot commented Apr 16, 2024

Uh oh!

Uh oh!

ManickaP Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

rzikm Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rzikm commented Apr 22, 2024

Uh oh!

azure-pipelines bot commented Apr 22, 2024

Uh oh!

Uh oh!

Uh oh!

stephentoub Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

rzikm Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ManickaP Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

rzikm Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ManickaP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CarnaViire left a comment

Choose a reason for hiding this comment

Uh oh!

rzikm commented Apr 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

rzikm commented Apr 16, 2024 •

edited

Loading