Reduce lock contention in socket on Windows

[Copilot]

Description

Lock contention in DynamicWinsockMethods: GetMethods() was called under a global lock(s_methodTable) on every socket's first async operation, serializing all concurrent callers even after the table was fully populated.

Changes

• DynamicWinsockMethods.cs — Replace List<T> + lock(s_methodTable) with a lock-free read path over a volatile DynamicWinsockMethods[]:

  • Hot path iterates the array with no lock (volatile read gives load-acquire; all elements are visible once the reference is seen)
  • Slow path acquires a dedicated Lock s_methodTableLock, double-checks under the lock, then publishes a new copy-on-write array via volatile store
  • Removes the lock(collection) antipattern; the old code locked on the List<T> instance itself

  // Before: every call (including reads) serialized on a single lock
  lock (s_methodTable) { /* linear scan + possible add */ }
  
  // After: reads are lock-free; writes are copy-on-write under a dedicated lock
  foreach (DynamicWinsockMethods methods in s_methodTable)   // volatile array read, no lock
      if (methods.Matches(...)) return methods;
  return GetMethodsSlow(...);                                 // lock only on miss

Original prompt

Use the pending changes to reduce lock contention in socket on windows

Created from Copilot CLI via the copilot delegate command.

---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

[dotnet-policy-service]

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

[stephentoub]

@copilot, revert the unrelated "rollup" changes

[Copilot]

@copilot, revert the unrelated "rollup" changes

Reverted in 5a6a0e0. The PR now only contains the DynamicWinsockMethods lock contention fix.

[Copilot]

Pull request overview

This PR reduces lock contention in the Windows socket async path by changing DynamicWinsockMethods.GetMethods from a globally locked List<T> to a copy-on-write volatile array with a lock-free read path.

Changes:

• Replace List<DynamicWinsockMethods> + lock(list) with volatile DynamicWinsockMethods[] for lock-free reads.
• Add a dedicated Lock (s_methodTableLock) and a slow path that double-checks and publishes a new array snapshot.

[stephentoub]

@copilot please invoke the code-review skill and post the analysis/results as a comment on this PR.

[Copilot]

@copilot please invoke the code-review skill and post the analysis/results as a comment on this PR.

🤖 Copilot Code Review — DynamicWinsockMethods lock contention fix

Holistic Assessment

Motivation: The original code acquired a global lock(s_methodTable) on every call to GetMethods() — including the overwhelmingly common read-hit path — serializing all concurrent socket-first-async-operation callers even after the table was fully populated. Given this is on the Windows socket hot path, removing contention for reads is justified.

Approach: The copy-on-write volatile array pattern is well-established in this codebase (e.g., ActivitySource.cs, ReferenceCountedProvidersManager). The change also fixes the lock(List<T>) antipattern (locking on a collection instance) by introducing a dedicated Lock object. The approach is appropriate given the table is tiny (≤4 entries typically) and append-only.

Summary: ✅ LGTM. The change is a clean, minimal, correct application of a well-known lock-free read / copy-on-write pattern. No correctness, thread-safety, or behavioral issues found.

---

Detailed Findings

✅ Thread safety — Volatile read + COW is correct

The fast path in GetMethods reads s_methodTable (a volatile field) once via foreach, which captures the array reference. Since the array is immutable once published and all elements are fully constructed (constructor sets readonly fields) before the volatile store in GetMethodsSlow makes the new array visible, readers always see a consistent snapshot. No torn reads or partially-initialized entries are possible.

✅ Double-check under lock — Correct

GetMethodsSlow re-reads s_methodTable inside the lock and re-scans before creating a new entry. This prevents duplicate entries when two threads race on the same (addressFamily, socketType, protocolType) combination.

✅ Lock antipattern fix — Good

The original lock(s_methodTable) locked on the List<T> instance itself, which is a well-known antipattern (external code could theoretically also lock on it, and it makes the intent unclear). The new dedicated Lock s_methodTableLock is correct.

✅ Copy-on-write mechanics — Correct

Array.Copy + publish-via-volatile-store is the standard copy-on-write pattern. The new entry is written to newTable[methodTable.Length] before the volatile store of s_methodTable = newTable, so all array elements are visible to subsequent readers.

✅ Matches helper — Clean refactoring

Extracting the 3-field comparison into Matches() avoids duplicating the comparison logic between the fast and slow paths. The method accesses only readonly instance fields, so it has no thread-safety concerns.

💡 Unused using System.Diagnostics — Minor

System.Diagnostics (for Debug) is still imported but Debug.Assert is not used anywhere in this file. This was also true before the PR so it's pre-existing, not introduced by this change. Consider removing in a follow-up.

✅ No behavioral change — No test gap

The change is purely an optimization of the synchronization strategy. The observable API contract (GetMethods returns the same instance for the same key tuple, instances are singletons) is preserved. No new tests are needed.

[stephentoub]

/ba-g unrelated wasm failures