Repository navigation
Trim per-request logger overhead - #4569
Conversation
Logger.GetLogger took a lock around a ConcurrentDictionary lookup on every call; use GetOrAdd. CanonicalizeResourcePathV2 built the arguments for a debug message (including a string concatenation of the endpoint) on every signed request; only build them when a debug logger is attached, via a new internal Logger.IsEnabled check.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open issues
ConcurrentDictionary.GetOrAddmay invoke the value factory multiple times for the same key under… · New Call sites that doif (IsEnabled(level)) DebugFormat(...)will enumerate_loggerstwice when… · New Inside the debug-enabled path,\" with endpoint \" + endpoint.AbsoluteUristill allocates an… · New
Review effort: Lite
Apply 2/3 GitHub Copilot feedback suggestions.
peterrsongg
left a comment
There was a problem hiding this comment.
looks good. thanks for the PR!
|
Internal build kicked off: 06086ecb-2cc5-464e-b4fc-8ba16b480ae0 |
|
Thanks for the contribution again, will merge it once the GitHub action runs and it'll be included in tomorrow's release. Also, if you're curious these are the benchmarks we set up: https://github.com/aws/aws-sdk-net/tree/main/sdk/test/Performance/SerdeBenchmarks (they're a bit more complex than "normal" BenchmarkDotNet but allow us to compare with other SDK languages). |


Description
Split out of #4566 as requested.
Two small per-request trims in logging:
Logger.GetLogger(Type)wrapped aConcurrentDictionarylookup in alockon every call. The dictionary already synchronizes, so the lookup now usesGetOrAddand no longer serializes every caller on the hot path.AWSSDKUtils.CanonicalizeResourcePathV2built the arguments for a debug message on every signed request, including a string concatenation of the endpoint, even though no debug logger is normally attached. A new internalLogger.IsEnabled(SdkLogLevel)check gates that work.Motivation and Context
Profiling showed
Logger.GetLogger(with its lock) and the debug-argument construction on the signing path of every request. On their own the two trims are below the noise floor of a single-threaded harness, so this PR is best read as a cleanup that removes a lock from a hot path rather than a measured throughput win; the lock removal matters more under concurrent load, which the harness does not exercise.Measured with a mocked-HTTP harness (no network) that drives a full client call through the pipeline: branch vs
development(9ab8dcf), three interleaved baseline/branch rounds of 20 × 2000 operations after a 6 s warm-up, process pinned to the P-cores of an i7-13700H, .NET 10 runtime against thenet8.0build of the SDK. Timings are the best round per side; allocations are exact (GC.GetAllocatedBytesForCurrentThread) and identical across rounds. Ratios are branch ÷ baseline.The only deterministic difference is 0.19 KB less per S3 PutObject (the debug-message arguments); all timing differences are within the ±3% run-to-run noise of this machine.
Benchmark code
The harness is not checked in. It is a console project that references the SDK projects directly, replaces the HTTP client with a canned-response handler, and measures a full client call through the pipeline. Build it once from the default branch and copy
bin/Release/net10.0toCoreBench-base, then build it from the branch and runab.sh "ddb sqs put list" 3.CoreBench.csprojBenchmarks.csMockHttp.csProgram.csab.shTesting
AWSSDK.Core.NetStandard.csprojbuilds for all targets with no warnings.net8.0test target compiles but could not be executed locally (MSTest adapter/framework mismatch in this environment), so CI is the first run of that target.AWSSDK.UnitTests.Coreonnet472, filtered toAWS4SignerandLogger: 31 passed.ClearLoggerCacheand adaptor registration still clear it) and the debug message is still emitted whenever a debug-enabled adaptor logger is registered.Dry-runs
Breaking Changes Assessment
Types of changes
Checklist
License
This change was generated by AI tools (Claude Code), and reviewed by Martin Costello (@martincostello).
🤖 Generated with Claude Code