Remove ConcurrentDictionary lookups from Unix socket event loop #109052
Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| /// <para>Instead, we reuse handles for new sockets. The race condition may therefore trigger a notification for the wrong socket, | ||
| /// but that is okay as those operations can be retried.</para> | ||
| /// </summary> | ||
| private static readonly ConcurrentQueue<GCHandle> s_gcHandles = new(); |
There was a problem hiding this comment.
Is this pool really necessary?
There was a problem hiding this comment.
It's not a typical pool (correctness more-so than perf). As the comment above describes, we're never freeing these handles since their lifetime is tricky.
This pool is here to avoid completely leaking a handle per socket, instead capping their number at whatever peak load the process reaches. That seems fine given that even with the dictionary, we wouldn't ever resize it back down.
There was a problem hiding this comment.
The dictionary has only a small overhead of memory per slot. What is the impact of never releasing a GCHandle?
There was a problem hiding this comment.
About 8 bytes * peak socket count.
I'm guessing the more meaningful impact would be on the GC throughput if you really have a ton of these laying around, though I'd be surprised if it mattered at these numbers.
When working on overall perf gains we should use the gold machines now, correct. I would suggest to use |
Running a bunch of different scenarios, this appears to be a slight improvement (<= 1%) on average, but that's below the run-to-run variance for these benchmarks.
@sebastienros is something like
scenarios/platform.benchmarks.yml --scenario plaintext --profile aspnet-gold-linreasonable for ASP.NET these days?cc: @tmds @benaadams