logger: avoid mutex contention#20454
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce logger mutex contention by removing the global synchronization wrapper from StreamHandler and instead adding locking only where it’s required (rotating file writes), with accompanying tests/bench-oriented checks to catch contention regressions.
Changes:
- Remove
SyncHandlerwrapping fromStreamHandlerto avoid global serialization during logging. - Add an internal mutex to
rotatingWriterto make rotation (stat/truncate/write/sync) safe under concurrency. - Add new tests in
bench_test.goto detect mutex contention and measure allocation/concurrency overhead.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
common/log/v3/handler.go |
Removes handler-level mutex from StreamHandler; adds mutex inside rotatingWriter.Write for safe multi-step rotation. |
common/log/v3/bench_test.go |
Adds contention-focused and allocation/concurrency tests around StreamHandler. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1fba234 to
1147a85
Compare
awskii
left a comment
There was a problem hiding this comment.
Approving this as better alternative to more complicated solution which also can silently drop logs in case of bursts. Sometimes we do burst logging, especially happens with Debug logging level. Usage allows blocking logging calls which is additional contention, i think this version would be easier to support
This reverts commit 02e843b.
This reverts commit 02e843b.
Reverts #20454 reason: gnosis has regression
an alternative for: #20148
Move mutex from log handler to file writer. Seems we used mutex in handler - to reduce allocs (by using shared buf in the past) - but now we don't use any shared buffer there.
TestStreamHandlerNoContention5K goroutines:5s -> 7ms