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

Calling Debug or Check on a Production Logger is slow #770

Closed
MarcErdmann opened this issue Dec 6, 2019 · 5 comments · Fixed by #771
Closed

Calling Debug or Check on a Production Logger is slow #770

MarcErdmann opened this issue Dec 6, 2019 · 5 comments · Fixed by #771

Comments

@MarcErdmann
Copy link

First of all: thank you for such a great library!
Unfortunately I have some performance issues when calling Check or Debug on a production logger. I am unsure if I am using the library wrong. I am using the following code:

package main

import (
	"os"
	"testing"

	log "github.com/sirupsen/logrus"
	"go.uber.org/zap"
)

func BenchmarkZap(b *testing.B) {
	logger, _ := zap.NewProduction()
	defer logger.Sync()

	for n := 0; n < b.N; n++ {
		logger.Debug("Test")
	}
}

func BenchmarkZapCheck(b *testing.B) {
	logger, _ := zap.NewProduction()
	defer logger.Sync()

	for n := 0; n < b.N; n++ {
		if ce := logger.Check(zap.DebugLevel, "Test"); ce != nil {
			ce.Write()
		}
	}
}

func BenchmarkLogrus(b *testing.B) {
	log.SetFormatter(&log.JSONFormatter{})
	log.SetOutput(os.Stdout)
	log.SetLevel(log.WarnLevel)

	for n := 0; n < b.N; n++ {
		log.Debug("Test")
	}
}

And I get the following results:

goos: linux
goarch: amd64
pkg: bachelorarbeit
BenchmarkZap-4        	15698260	        74.8 ns/op	       0 B/op	       0 allocs/op
BenchmarkZapCheck-4   	17619888	        68.1 ns/op	       0 B/op	       0 allocs/op
BenchmarkLogrus-4     	341667280	         3.54 ns/op	       0 B/op	       0 allocs/op

Is this performance expected?

@abhinav
Copy link
Collaborator

abhinav commented Dec 6, 2019

Hello! No, you're not using Zap incorrectly. Logrus is misconfigured in that
example. Logs are being emitted at the debug level but the logger is
configured to discard anything below Warn.

After fixing the configuration, we get a more accurate idea of logging
performance.

func BenchmarkLogrus(b *testing.B) {
  log.SetFormatter(&log.JSONFormatter{})
  log.SetOutput(ioutil.Discard)
- log.SetLevel(log.WarnLevel)
+ log.SetLevel(log.DebugLevel)
BenchmarkZap-4             12965005                88.3 ns/op
BenchmarkZapCheck-4        13573184                88.3 ns/op
BenchmarkLogrus-4            453739              2583 ns/op

Hope that helped!

@abhinav abhinav closed this as completed Dec 6, 2019
@MarcErdmann
Copy link
Author

Thanks for your reply. To discard the logs was my intention actually. The CPU profile of my production application shows that zap uses 20% of my application's CPU time. Although I always use the method "check" and the production configuration. Logrus seems to be 20 times faster discarding debug logs in a production environment. Is there a reason for this?

@abhinav
Copy link
Collaborator

abhinav commented Dec 9, 2019

Oops, you're right. I misread the example.

@abhinav abhinav reopened this Dec 9, 2019
@abhinav
Copy link
Collaborator

abhinav commented Dec 9, 2019

Dug into this. Most of the time is spent on time.Now. Looks like there was
prior discussion on dropping the time.Now call in #664, but it's unavoidable
until 2.0.

prashantv added a commit that referenced this issue Dec 10, 2019
Calling `time.Now()` and creating an entry is unnecessary
if the underlying core has the specified level disabled.

To reduce the cost of logs at disabled levels, skip entry
creation if the log level is disabled in the core.

This special logic does not apply to DPanic or higher logs as
they may need to panic/exit even if the entry does not cause
any log to be emitted.

On my machine, disabled debugging logs are 6x (~60ns to ~10ns).

Fixes #770.
@prashantv
Copy link
Collaborator

One option: the Core embeds LevelEnabler, so it's quite reasonable to check the level before we create an entry and call Check on the underlying core. See PR: #771

The above benchmark, before:

BenchmarkZap-12                 16877612                60.0 ns/op
BenchmarkZapCheck-12            19379218                59.7 ns/op
BenchmarkLogrus-12              416875682                2.92 ns/op

and after:

BenchmarkZap-12                 113397655               10.2 ns/op
BenchmarkZapCheck-12            156923647                7.60 ns/op
BenchmarkLogrus-12              411759578                2.96 ns/op

Zap is still slower, likely because:

  • We use an atomic for the level so it can be changed dynamically. The user can create their own logger with a custom LevelEnabler which isn't an atomic, that should be a little faster.
  • We lose inlining due to the interfaces.

prashantv added a commit that referenced this issue Dec 18, 2019
Calling `time.Now()` and creating an entry is unnecessary
if the underlying core has the specified level disabled.

To reduce the cost of logs at disabled levels, skip entry
creation if the log level is disabled in the core.

This special logic does not apply to DPanic or higher logs as
they may need to panic/exit even if the entry does not cause
any log to be emitted.

On my machine, disabled debugging logs are 6x (~60ns to ~10ns)
based on the example in the issue.

benchcmp:
```
benchmark                                                 old ns/op     new ns/op     delta
BenchmarkDisabledWithoutFields/Zap-12                     8.42          1.59          -81.12%
BenchmarkDisabledWithoutFields/Zap.Check-12               8.01          1.32          -83.52%
BenchmarkDisabledWithoutFields/Zap.Sugar-12               12.4          11.4          -8.06%
BenchmarkDisabledWithoutFields/Zap.SugarFormatting-12     117           102           -12.82%
```

Fixes #770.
cgxxv pushed a commit to cgxxv/zap that referenced this issue Mar 25, 2022
Calling `time.Now()` and creating an entry is unnecessary
if the underlying core has the specified level disabled.

To reduce the cost of logs at disabled levels, skip entry
creation if the log level is disabled in the core.

This special logic does not apply to DPanic or higher logs as
they may need to panic/exit even if the entry does not cause
any log to be emitted.

On my machine, disabled debugging logs are 6x (~60ns to ~10ns)
based on the example in the issue.

benchcmp:
```
benchmark                                                 old ns/op     new ns/op     delta
BenchmarkDisabledWithoutFields/Zap-12                     8.42          1.59          -81.12%
BenchmarkDisabledWithoutFields/Zap.Check-12               8.01          1.32          -83.52%
BenchmarkDisabledWithoutFields/Zap.Sugar-12               12.4          11.4          -8.06%
BenchmarkDisabledWithoutFields/Zap.SugarFormatting-12     117           102           -12.82%
```

Fixes uber-go#770.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants