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

log: avoid stack lookups when not needed/used #28069

Merged
merged 7 commits into from
Sep 7, 2023
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
12 changes: 12 additions & 0 deletions common/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"reflect"
"strings"
"testing"
"time"
)

func TestBytesConversion(t *testing.T) {
Expand Down Expand Up @@ -583,3 +584,14 @@ func TestAddressEIP55(t *testing.T) {
t.Fatal("Unexpected address after unmarshal")
}
}

func BenchmarkPrettyDuration(b *testing.B) {
var x = PrettyDuration(time.Duration(int64(1203123912312)))
b.Logf("Pre %s", time.Duration(x).String())
var a string
b.ResetTimer()
for i := 0; i < b.N; i++ {
a = x.String()
}
b.Logf("Post %s", a)
}
8 changes: 8 additions & 0 deletions log/format.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,16 @@ var locationTrims = []string{
// format output.
func PrintOrigins(print bool) {
locationEnabled.Store(print)
if print {
stackEnabled.Store(true)
}
}

// stackEnabled is an atomic flag controlling whether the log handler needs
// to store the callsite stack. This is needed in case any handler wants to
// print locations (locationEnabled), use vmodule, or print full stacks (BacktraceAt).
var stackEnabled atomic.Bool
Copy link
Member

Choose a reason for hiding this comment

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

linter's gonna lint :P field and comment name don't match


// locationEnabled is an atomic flag controlling whether the terminal formatter
// should append the log locations too when printing entries.
var locationEnabled atomic.Bool
Expand Down
8 changes: 6 additions & 2 deletions log/handler_glog.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,10 @@ func (h *GlogHandler) Vmodule(ruleset string) error {
h.patterns = filter
h.siteCache = make(map[uintptr]Lvl)
h.override.Store(len(filter) != 0)

// Enable location storage (globally)
if len(h.patterns) > 0 {
stackEnabled.Store(true)
}
return nil
}

Expand Down Expand Up @@ -172,7 +175,8 @@ func (h *GlogHandler) BacktraceAt(location string) error {

h.location = location
h.backtrace.Store(len(location) > 0)

// Enable location storage (globally)
stackEnabled.Store(true)
return nil
}

Expand Down
9 changes: 6 additions & 3 deletions log/logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -177,19 +177,22 @@ type logger struct {
}

func (l *logger) write(msg string, lvl Lvl, ctx []interface{}, skip int) {
l.h.Log(&Record{
record := &Record{
Time: time.Now(),
Lvl: lvl,
Msg: msg,
Ctx: newContext(l.ctx, ctx),
Call: stack.Caller(skip),
KeyNames: RecordKeyNames{
Time: timeKey,
Msg: msgKey,
Lvl: lvlKey,
Ctx: ctxKey,
},
})
}
if stackEnabled.Load() {
record.Call = stack.Caller(skip)
Copy link
Member

@rjl493456442 rjl493456442 Sep 7, 2023

Choose a reason for hiding this comment

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

It's problematic. e.g. in the log/handler_glog.go, L188, we assume this is available.

But I agree it's a good optimization, but just need more checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, interesting. I dug into this, it turns out that r.Call.String() == %!v(NOFUNC).
due to

// String implements fmt.Stinger. It is equivalent to fmt.Sprintf("%v", c).
func (c Call) String() string {
	return fmt.Sprint(c)
}

So there's no crash, just it won't print the backtrace.

Copy link
Contributor Author

@holiman holiman Sep 7, 2023

Choose a reason for hiding this comment

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

Should be noted, though: the Call is stack.Call -- not a pointer. Nothing is nil here. And it goes on

type Call struct {
	frame runtime.Frame
}

(no pointers)

type Frame struct {
	// PC is the program counter for the location in this frame.
	// For a frame that calls another frame, this will be the
	// program counter of a call instruction. Because of inlining,
	// multiple frames may have the same PC value, but different
	// symbolic information.
	PC uintptr
...

This struct too is (nearly) pointer-free.

Copy link
Member

Choose a reason for hiding this comment

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

Ha I spent two hours yesterday trying to create a test to hit this, but couldn't because no pointer. So it looks good to me

}
l.h.Log(record)
}

func (l *logger) New(ctx ...interface{}) Logger {
Expand Down
67 changes: 67 additions & 0 deletions log/logger_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package log

import (
"bytes"
"os"
"strings"
"testing"
)

// TestLoggingWithTrace checks that if BackTraceAt is set, then the
// gloghandler is capable of spitting out a stacktrace
func TestLoggingWithTrace(t *testing.T) {
defer stackEnabled.Store(stackEnabled.Load())
out := new(bytes.Buffer)
logger := New()
{
glog := NewGlogHandler(StreamHandler(out, TerminalFormat(false)))
glog.Verbosity(LvlTrace)
if err := glog.BacktraceAt("logger_test.go:24"); err != nil {
t.Fatal(err)
}
logger.SetHandler(glog)
}
logger.Trace("a message", "foo", "bar") // Will be bumped to INFO
have := out.String()
if !strings.HasPrefix(have, "INFO") {
t.Fatalf("backtraceat should bump level to info: %s", have)
}
// The timestamp is locale-dependent, so we want to trim that off
// "INFO [01-01|00:00:00.000] a messag ..." -> "a messag..."
have = strings.Split(have, "]")[1]
wantPrefix := " a message\n\ngoroutine"
if !strings.HasPrefix(have, wantPrefix) {
t.Errorf("\nhave: %q\nwant: %q\n", have, wantPrefix)
}
}

// TestLoggingWithVmodule checks that vmodule works.
func TestLoggingWithVmodule(t *testing.T) {
defer stackEnabled.Store(stackEnabled.Load())
out := new(bytes.Buffer)
logger := New()
{
glog := NewGlogHandler(StreamHandler(out, TerminalFormat(false)))
glog.Verbosity(LvlCrit)
logger.SetHandler(glog)
logger.Warn("This should not be seen", "ignored", "true")
glog.Vmodule("logger_test.go=5")
}
logger.Trace("a message", "foo", "bar")
have := out.String()
// The timestamp is locale-dependent, so we want to trim that off
// "INFO [01-01|00:00:00.000] a messag ..." -> "a messag..."
have = strings.Split(have, "]")[1]
want := " a message foo=bar\n"
if have != want {
t.Errorf("\nhave: %q\nwant: %q\n", have, want)
}
}

func BenchmarkTraceLogging(b *testing.B) {
Root().SetHandler(LvlFilterHandler(LvlInfo, StreamHandler(os.Stderr, TerminalFormat(true))))
b.ResetTimer()
for i := 0; i < b.N; i++ {
Trace("a message", "v", i)
}
}