Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce allocations performed by the Logger returned from log.With. #28

Merged
merged 2 commits into from
Apr 20, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 33 additions & 8 deletions log/log.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,32 @@ func With(logger Logger, keyvals ...interface{}) Logger {
if w, ok := logger.(Wither); ok {
return w.With(keyvals...)
}
return LoggerFunc(func(kvs ...interface{}) error {
// Limiting the capacity of the first argument to append ensures that
// a new backing array is created if the slice must grow. Using the
// extra capacity without copying risks a data race that would violate
// the Logger interface contract.
n := len(keyvals)
return logger.Log(append(keyvals[:n:n], kvs...)...)
})
// Limiting the capacity of the stored keyvals ensures that a new
// backing array is created if the slice must grow in Log or With.
// Using the extra capacity without copying risks a data race that
// would violate the Logger interface contract.
n := len(keyvals)
return &withLogger{
logger: logger,
keyvals: keyvals[:n:n],
}
}

type withLogger struct {
logger Logger
keyvals []interface{}
}

func (l *withLogger) Log(keyvals ...interface{}) error {
return l.logger.Log(append(l.keyvals, keyvals...)...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this thread safe?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, l.keyvals is capacity sliced when each withLogger is created. See line 33 and 50. The code passes TestWithConcurrent with race detector and multiple cores enabled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

On Sunday, April 19, 2015, Chris Hines [email protected] wrote:

In log/log.go
#28 (comment):

  • // Using the extra capacity without copying risks a data race that
  • // would violate the Logger interface contract.
  • n := len(keyvals)
  • return &withLogger{
  •   logger:  logger,
    
  •   keyvals: keyvals[:n:n],
    
  • }
    +}

+type withLogger struct {

  • logger Logger
  • keyvals []interface{}
    +}

+func (l *withLogger) Log(keyvals ...interface{}) error {

  • return l.logger.Log(append(l.keyvals, keyvals...)...)

Yes, l.keyvals is capacity sliced when each withLogger is created. See
line 33 and 50. The code passes TestWithConcurrent with race detector and
multiple cores enabled.


Reply to this email directly or view it on GitHub
https://github.com/peterbourgon/gokit/pull/28/files#r28652359.

Sent from Gmail Mobile

}

func (l *withLogger) With(keyvals ...interface{}) Logger {
n := len(l.keyvals) + len(keyvals)
return &withLogger{
logger: l.logger,
keyvals: append(l.keyvals, keyvals...)[:n:n],
}
}

// LoggerFunc is an adapter to allow use of ordinary functions as Loggers. If
Expand All @@ -49,3 +67,10 @@ func (f LoggerFunc) Log(keyvals ...interface{}) error {
type Wither interface {
With(keyvals ...interface{}) Logger
}

// NewDiscardLogger returns a logger that does not log anything.
func NewDiscardLogger() Logger {
return LoggerFunc(func(...interface{}) error {
return nil
})
}
43 changes: 43 additions & 0 deletions log/log_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,3 +78,46 @@ func TestWithConcurrent(t *testing.T) {
}
}
}

func BenchmarkDiscard(b *testing.B) {
logger := log.NewDiscardLogger()
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
logger.Log("k", "v")
}
}

func BenchmarkOneWith(b *testing.B) {
logger := log.NewDiscardLogger()
logger = log.With(logger, "k", "v")
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
logger.Log("k", "v")
}
}

func BenchmarkTwoWith(b *testing.B) {
logger := log.NewDiscardLogger()
for i := 0; i < 2; i++ {
logger = log.With(logger, "k", "v")
}
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
logger.Log("k", "v")
}
}

func BenchmarkTenWith(b *testing.B) {
logger := log.NewDiscardLogger()
for i := 0; i < 10; i++ {
logger = log.With(logger, "k", "v")
}
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
logger.Log("k", "v")
}
}