Skip to content

Commit

Permalink
log: fix remaining misuse of runtime.Callers/runtime.FuncForPC
Browse files Browse the repository at this point in the history
Fixes #17770.

This commit fixes the last user of `runtime.Callers` that misused
the stdlib function by translating the PC values it returned directly
into symbolic information (see https://golang.org/pkg/runtime/#Callers).
Go's documentation warns that this is a recipe for disaster when mixed
with mid-stack inlining.

The other concern in #17770 was this comment:
#17770 (comment).
This was discussed in golang/go#29582 and
addressed in golang/go@956879d.

An alternative would be to use `runtime.Caller` here, but that would
force an allocation that was hard-earned in #29017. Instead, this commit
avoids any performance hit.

```
name                 old time/op    new time/op    delta
Header-4                267ns ± 1%     268ns ± 0%    ~     (p=0.584 n=10+10)
VDepthWithVModule-4     260ns ± 3%     255ns ± 1%  -1.87%  (p=0.018 n=10+9)

name                 old alloc/op   new alloc/op   delta
Header-4                0.00B          0.00B         ~     (all equal)
VDepthWithVModule-4     0.00B          0.00B         ~     (all equal)

name                 old allocs/op  new allocs/op  delta
Header-4                 0.00           0.00         ~     (all equal)
VDepthWithVModule-4      0.00           0.00         ~     (all equal)
```

Release note: None
  • Loading branch information
nvanbenschoten committed Feb 26, 2019
1 parent 7013956 commit 6654c81
Showing 1 changed file with 6 additions and 6 deletions.
12 changes: 6 additions & 6 deletions pkg/util/log/clog.go
Original file line number Diff line number Diff line change
Expand Up @@ -1391,9 +1391,9 @@ func NewStdLogger(severity Severity) *stdLog.Logger {
// of its .go suffix, and uses filepath.Match, which is a little more
// general than the *? matching used in C++.
// l.mu is held.
func (l *loggingT) setV(pc uintptr) level {
fn := runtime.FuncForPC(pc)
file, _ := fn.FileLine(pc)
func (l *loggingT) setV(pc [1]uintptr) level {
frame, _ := runtime.CallersFrames(pc[:]).Next()
file := frame.File
// The file is something like /a/b/c/d.go. We want just the d.
if strings.HasSuffix(file, ".go") {
file = file[:len(file)-3]
Expand All @@ -1403,11 +1403,11 @@ func (l *loggingT) setV(pc uintptr) level {
}
for _, filter := range l.vmodule.filter {
if filter.match(file) {
l.vmap[pc] = filter.level
l.vmap[pc[0]] = filter.level
return filter.level
}
}
l.vmap[pc] = 0
l.vmap[pc[0]] = 0
return 0
}

Expand Down Expand Up @@ -1477,7 +1477,7 @@ func VDepth(l int32, depth int) bool {
logging.mu.Lock()
v, ok := logging.vmap[pcs[0]]
if !ok {
v = logging.setV(pcs[0])
v = logging.setV(pcs)
}
logging.mu.Unlock()
logging.pcsPool.Put(poolObj)
Expand Down

0 comments on commit 6654c81

Please sign in to comment.