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

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

/azp run runtime

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

Azure Pipelines successfully started running 1 pipeline(s).

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

    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.

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

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).

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

+
+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) &&

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

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

ManickaP

Nits, otherwise LGTM.

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

Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs Outdated

Apply suggestions from code review

45bbf45

hamarb123 reviewed Apr 16, 2024

View reviewed changes

Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/msquic.cs Outdated

Remove ReadOnlySpan property

8f5ae6d

ManickaP reviewed Apr 17, 2024

View reviewed changes

Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs Outdated

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

Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs Outdated

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

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

stephentoub reviewed Apr 22, 2024

View reviewed changes

Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ReceiveBuffers.cs Outdated

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

Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/QUIC_SETTINGS.IEquattable.cs Outdated

Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/QUIC_SETTINGS.IEquattable.cs Outdated

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

Navigation Menu

Uh oh!

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 merged 9 commits into
dotnet:mainfrom
rzikm:quic-no-defensive-copy

rzikm commented Apr 16, 2024 •

edited

Loading

dotnet-policy-service Bot commented Apr 16, 2024

MihaZupan commented Apr 16, 2024

MihaZupan left a comment

Uh oh!

rzikm commented Apr 16, 2024

azure-pipelines Bot commented Apr 16, 2024

Uh oh!

ManickaP Apr 17, 2024

rzikm Apr 17, 2024

Uh oh!

Uh oh!

Uh oh!

rzikm commented Apr 22, 2024

azure-pipelines Bot commented Apr 22, 2024

Uh oh!

Uh oh!

stephentoub Apr 22, 2024

rzikm Apr 22, 2024

Uh oh!

ManickaP Apr 22, 2024

rzikm Apr 22, 2024

Uh oh!

ManickaP left a comment

Uh oh!

Uh oh!

CarnaViire left a comment

rzikm commented Apr 25, 2024

Labels

8 participants

Uh oh!

Conversation

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

Uh oh!

dotnet-policy-service Bot commented Apr 16, 2024

MihaZupan commented Apr 16, 2024

MihaZupan left a comment

Choose a reason for hiding this comment

Uh oh!

rzikm commented Apr 16, 2024

azure-pipelines Bot commented Apr 16, 2024

Uh oh!

ManickaP Apr 17, 2024

Choose a reason for hiding this comment

rzikm Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rzikm commented Apr 22, 2024

azure-pipelines Bot commented Apr 22, 2024

Uh oh!

Uh oh!

stephentoub Apr 22, 2024

Choose a reason for hiding this comment

rzikm Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

ManickaP Apr 22, 2024

Choose a reason for hiding this comment

rzikm Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

ManickaP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CarnaViire left a comment

Choose a reason for hiding this comment

rzikm commented Apr 25, 2024

Labels

8 participants

rzikm commented Apr 16, 2024 •

edited

Loading