Skip to content

Commit aa07770

Browse files
rzikmManickaP
rzikm
and
ManickaP
authored
Avoid some defensive copies on readonly structs in System.Net.Quic (#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>
1 parent 56048b5 commit aa07770

File tree

4 files changed

+170
-2
lines changed

4 files changed

+170
-2
lines changed

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

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

9393
/// <summary>
9494
/// Optional parent safe handle, used to increment/decrement reference count with the lifetime of this instance.

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

+94
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System;
5+
6+
namespace Microsoft.Quic;
7+
8+
internal partial struct QUIC_SETTINGS : System.IEquatable<QUIC_SETTINGS>
9+
{
10+
// Because QUIC_SETTINGS may contain gaps due to layout/alignment of individual
11+
// fields, we implement IEquatable<QUIC_SETTINGS> manually. If a new field is added,
12+
// then there is a unit test which should fail.
13+
14+
public readonly bool Equals(QUIC_SETTINGS other)
15+
{
16+
return Anonymous1.IsSetFlags == other.Anonymous1.IsSetFlags
17+
&& MaxBytesPerKey == other.MaxBytesPerKey
18+
&& HandshakeIdleTimeoutMs == other.HandshakeIdleTimeoutMs
19+
&& IdleTimeoutMs == other.IdleTimeoutMs
20+
&& MtuDiscoverySearchCompleteTimeoutUs == other.MtuDiscoverySearchCompleteTimeoutUs
21+
&& TlsClientMaxSendBuffer == other.TlsClientMaxSendBuffer
22+
&& TlsServerMaxSendBuffer == other.TlsServerMaxSendBuffer
23+
&& StreamRecvWindowDefault == other.StreamRecvWindowDefault
24+
&& StreamRecvBufferDefault == other.StreamRecvBufferDefault
25+
&& ConnFlowControlWindow == other.ConnFlowControlWindow
26+
&& MaxWorkerQueueDelayUs == other.MaxWorkerQueueDelayUs
27+
&& MaxStatelessOperations == other.MaxStatelessOperations
28+
&& InitialWindowPackets == other.InitialWindowPackets
29+
&& SendIdleTimeoutMs == other.SendIdleTimeoutMs
30+
&& InitialRttMs == other.InitialRttMs
31+
&& MaxAckDelayMs == other.MaxAckDelayMs
32+
&& DisconnectTimeoutMs == other.DisconnectTimeoutMs
33+
&& KeepAliveIntervalMs == other.KeepAliveIntervalMs
34+
&& CongestionControlAlgorithm == other.CongestionControlAlgorithm
35+
&& PeerBidiStreamCount == other.PeerBidiStreamCount
36+
&& PeerUnidiStreamCount == other.PeerUnidiStreamCount
37+
&& MaxBindingStatelessOperations == other.MaxBindingStatelessOperations
38+
&& StatelessOperationExpirationMs == other.StatelessOperationExpirationMs
39+
&& MinimumMtu == other.MinimumMtu
40+
&& MaximumMtu == other.MaximumMtu
41+
&& _bitfield == other._bitfield
42+
&& MaxOperationsPerDrain == other.MaxOperationsPerDrain
43+
&& MtuDiscoveryMissingProbeCount == other.MtuDiscoveryMissingProbeCount
44+
&& DestCidUpdateIdleTimeoutMs == other.DestCidUpdateIdleTimeoutMs
45+
&& Anonymous2.Flags == other.Anonymous2.Flags
46+
&& StreamRecvWindowBidiLocalDefault == other.StreamRecvWindowBidiLocalDefault
47+
&& StreamRecvWindowBidiRemoteDefault == other.StreamRecvWindowBidiRemoteDefault
48+
&& StreamRecvWindowUnidiDefault == other.StreamRecvWindowUnidiDefault;
49+
}
50+
51+
public override readonly int GetHashCode()
52+
{
53+
HashCode hash = default;
54+
hash.Add(Anonymous1.IsSetFlags);
55+
hash.Add(MaxBytesPerKey);
56+
hash.Add(HandshakeIdleTimeoutMs);
57+
hash.Add(IdleTimeoutMs);
58+
hash.Add(MtuDiscoverySearchCompleteTimeoutUs);
59+
hash.Add(TlsClientMaxSendBuffer);
60+
hash.Add(TlsServerMaxSendBuffer);
61+
hash.Add(StreamRecvWindowDefault);
62+
hash.Add(StreamRecvBufferDefault);
63+
hash.Add(ConnFlowControlWindow);
64+
hash.Add(MaxWorkerQueueDelayUs);
65+
hash.Add(MaxStatelessOperations);
66+
hash.Add(InitialWindowPackets);
67+
hash.Add(SendIdleTimeoutMs);
68+
hash.Add(InitialRttMs);
69+
hash.Add(MaxAckDelayMs);
70+
hash.Add(DisconnectTimeoutMs);
71+
hash.Add(KeepAliveIntervalMs);
72+
hash.Add(CongestionControlAlgorithm);
73+
hash.Add(PeerBidiStreamCount);
74+
hash.Add(PeerUnidiStreamCount);
75+
hash.Add(MaxBindingStatelessOperations);
76+
hash.Add(StatelessOperationExpirationMs);
77+
hash.Add(MinimumMtu);
78+
hash.Add(MaximumMtu);
79+
hash.Add(_bitfield);
80+
hash.Add(MaxOperationsPerDrain);
81+
hash.Add(MtuDiscoveryMissingProbeCount);
82+
hash.Add(DestCidUpdateIdleTimeoutMs);
83+
hash.Add(Anonymous2.Flags);
84+
hash.Add(StreamRecvWindowBidiLocalDefault);
85+
hash.Add(StreamRecvWindowBidiRemoteDefault);
86+
hash.Add(StreamRecvWindowUnidiDefault);
87+
return hash.ToHashCode();
88+
}
89+
90+
public override readonly bool Equals(object? obj)
91+
{
92+
return obj is QUIC_SETTINGS other && Equals(other);
93+
}
94+
}

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ namespace Microsoft.Quic
2020
{
2121
internal unsafe partial struct QUIC_BUFFER
2222
{
23-
public Span<byte> Span => new(Buffer, (int)Length);
23+
public readonly Span<byte> Span => new(Buffer, (int)Length);
2424
}
2525

2626
internal partial class MsQuic

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

+74
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System;
5+
using System.Diagnostics;
6+
using System.Net.Security;
7+
using System.Threading.Tasks;
8+
using System.Runtime.InteropServices;
9+
using System.Reflection;
10+
using System.Linq;
11+
using Xunit;
12+
using Xunit.Abstractions;
13+
14+
using Microsoft.Quic;
15+
16+
namespace System.Net.Quic.Tests
17+
{
18+
public class MsQuicInteropTests
19+
{
20+
private static MemberInfo[] GetMembers<T>()
21+
{
22+
var members = typeof(T).FindMembers(MemberTypes.Field | MemberTypes.Property, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.Public, (mi, _) =>
23+
{
24+
if (mi is PropertyInfo property && property.GetSetMethod() == null)
25+
{
26+
return false;
27+
}
28+
29+
return true;
30+
}, null);
31+
32+
Assert.NotEmpty(members);
33+
34+
return members;
35+
}
36+
37+
private static void ResetMember(MemberInfo member, object instance)
38+
{
39+
switch (member)
40+
{
41+
case FieldInfo field:
42+
field.SetValue(instance, Activator.CreateInstance(field.FieldType));
43+
break;
44+
case PropertyInfo property:
45+
property.SetValue(instance, Activator.CreateInstance(property.PropertyType));
46+
break;
47+
default:
48+
throw new InvalidOperationException($"Unexpected member type: {member.MemberType}");
49+
}
50+
}
51+
52+
[Fact]
53+
public void QuicSettings_Equals_RespectsAllMembers()
54+
{
55+
QUIC_SETTINGS settings = new QUIC_SETTINGS();
56+
57+
// make sure the extension definition is included in compilation
58+
Assert.Contains(typeof(IEquatable<QUIC_SETTINGS>), typeof(QUIC_SETTINGS).GetInterfaces());
59+
60+
var settingsSpan = MemoryMarshal.AsBytes(new Span<QUIC_SETTINGS>(ref settings));
61+
62+
// Fill the memory with 1s,we will try to zero out each member and compare
63+
settingsSpan.Fill(0xFF);
64+
65+
foreach (MemberInfo member in GetMembers<QUIC_SETTINGS>())
66+
{
67+
// copy and box the instance because reflection methods take a reference type arg
68+
object boxed = settings;
69+
ResetMember(member, boxed);
70+
Assert.False(settings.Equals((QUIC_SETTINGS)boxed), $"Member {member.Name} is not compared.");
71+
}
72+
}
73+
}
74+
}

0 commit comments

Comments
 (0)