Repository navigation
perf(logger): build default log line without fmt.Sprintf - #4925
Open
TheNetherWatcher wants to merge 1 commit into
Open
TheNetherWatcher wants to merge 1 commit into
TheNetherWatcher wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4925 +/- ##
==========================================
+ Coverage 98.65% 98.72% +0.07%
==========================================
Files 48 48
Lines 3188 3220 +32
==========================================
+ Hits 3145 3179 +34
+ Misses 43 41 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
defaultLogFormatterbuilds every request log line with onefmt.Sprintfcall (13 args, padding verbs,%#vfor the path). That costs about a dozen allocations per request, mostly from boxing the arguments intointerface{}and the extraFormat/Stringresults.This change appends the same fields into a byte slice instead:
time.AppendFormatfor the timestamp andstrconv.AppendIntfor the status codeappendPaddedhelper for the%8v,%15sand%-7sfields. It counts runes like fmt does, soµslatencies and non-ASCII values pad the same waystrconv.AppendQuotefor the path, which is what%#vdoes for a stringThe output is byte-for-byte the same. To check that, I ran a throwaway test comparing the new function with the old
Sprintfversion over 48,600 combinations: color on and off, negative, short and 5-digit status codes, negative/µs/ms/hour latencies, IPv6 and non-ASCII client IPs, long and non-ASCII methods, and paths containing quotes, backslashes, control bytes, invalid UTF-8 and backticks. Every case matched.Benchmarks
Apple M-series, Go 1.27.2. Runs alternated old/new, 6 each, medians shown. The machine was under load (load avg around 7), so the ns/op numbers are noisy. The alloc counts are exact.
BenchmarkLoggerMiddleware(existing)defaultLogFormatteralone (newBenchmarkDefaultLogFormatter)Tests
TestDefaultLogFormatterPadding, which covers rune-counted padding (25µs), values longer than their field width (an IPv6 address,PROPFIND), status0, quoting of non-ASCII and control characters in the path, and an error message. It passes on unmodifiedmastertoo, so it pins the existing output.BenchmarkDefaultLogFormatter.go test ./...,go test -raceon the logger tests,go vet ./...andgolangci-lint runall pass. The lint findings it reports are already onmasterand none are inlogger.go.Note: #4652 also edits
logger.go. It changes only thefmt.Fprintcall inLoggerWithConfig, so the two don't overlap in the diff. But that call is the last use offmtin the file after this change, so whichever PR merges second needs to drop thefmtimport.