For me personally, it is to avoid that someone in the future will do heavy things just for the log before passing in the log parameter, but forget to add an additional guard statement, a common use case, to record structed json log

log.Debugf("%s", toJson(request))

At this time, the guard statement in the log method is invalid

Thanks for your reply. @jizhuozhi
I agree with you, just like the code below, we need to make a judgment before printing the log:

	if log.DefaultLogger.GetLogLevel() >= log.DEBUG {
		_, isReq := cmd.(*Request)
		_, isResp := cmd.(*Response)
		if cmd != nil {
			log.DefaultLogger.Debugf("decode command, contextId: %d, req(%v)|resp(%v)|hb(%v), rpc id: %d \n", contextId, isReq, isResp, cmd.IsHeartbeatFrame(), cmd.GetRequestId())
		}
	}

However, I think that in most scenarios we don't need this judgment, which can make the code cleaner and more functional.

Thanks for your reply. @jizhuozhi I agree with you, just like the code below, we need to make a judgment before printing the log:

	if log.DefaultLogger.GetLogLevel() >= log.DEBUG {
		_, isReq := cmd.(*Request)
		_, isResp := cmd.(*Response)
		if cmd != nil {
			log.DefaultLogger.Debugf("decode command, contextId: %d, req(%v)|resp(%v)|hb(%v), rpc id: %d \n", contextId, isReq, isResp, cmd.IsHeartbeatFrame(), cmd.GetRequestId())
		}
	}

However, I think that in most scenarios we don't need this judgment, which can make the code cleaner and more functional.

Another use is to prevent args from mem escaping.
like isReq and isResp in your example.

tmp point$ cat test.go
package main

import (
	"mosn.io/mosn/pkg/protocol/xprotocol/bolt"
	"mosn.io/pkg/log"
)

func main() {
	var cmd interface{}
	cmd = &bolt.Request{}
	_, isReq := cmd.(*bolt.Request)
	_, isResp := cmd.(*bolt.Response)
	if cmd != nil {
		log.DefaultLogger.Debugf("decode command, req(%v)|resp(%v)|hb(%v)\n", isReq, isResp)
	}
}
tmp point$ go build -gcflags=-m test.go
# command-line-arguments
./test.go:10:8: &bolt.Request{} does not escape
./test.go:14:27: ... argument escapes to heap
./test.go:14:73: isReq escapes to heap
./test.go:14:80: isResp escapes to heap
tmp point$ cat test.go
package main

import (
	"mosn.io/mosn/pkg/protocol/xprotocol/bolt"
	"mosn.io/pkg/log"
)

func main() {
	var cmd interface{}
	cmd = &bolt.Request{}
	if false {
		_, isReq := cmd.(*bolt.Request)
		_, isResp := cmd.(*bolt.Response)
		if cmd != nil {
			log.DefaultLogger.Debugf("decode command, req(%v)|resp(%v)|hb(%v)\n", isReq, isResp)
		}
	}
}
tmp point$ go build -gcflags=-m test.go
# command-line-arguments
./test.go:8:6: can inline main
./test.go:10:8: &bolt.Request{} does not escape

Good point, your case is actually similar to the first one @jizhuozhi provided, which is essentially to avoid allocate resources in non-debug level, which makes sense, but here are something can be discussed:

1. about the resources to be allocated:

   • If what needed is computational resource (such as toJson(request)), we may pass a closure in through the logger to avoid pre-compute, like: log.Debug("%s", func() { toJson(request) })
   • If what needed is memory resources, maybe pass a value rather than pointer is better, which heap allocation could be avoided

2. about your case:
   The if false {...} may hint the compiler to safely skipped all logic inside it. If we put some dynamically changing value into the condition, then the pointer will still escape out:

➜  mosn git:(master) ✗ cat tmp.go 
package main

import (
        "mosn.io/mosn/pkg/protocol/xprotocol/bolt"
        "mosn.io/pkg/log"
)

var condition = false

func main() {
        var cmd interface{}
        cmd = &bolt.Request{}
        if condition {
                _, isReq := cmd.(*bolt.Request)
                _, isResp := cmd.(*bolt.Response)
                if cmd != nil {
                     log.DefaultLogger.Debugf("decode command, req(%v)|resp(%v)|hb(%v)\n", func() { toJson() }, isResp)
                }
        }
}
➜  mosn git:(master) ✗ go build -gcflags=-m tmp.go
# command-line-arguments
./tmp.go:12:8: &bolt.Request{} does not escape
./tmp.go:17:28: ... argument escapes to heap
./tmp.go:17:29: isReq escapes to heap
./tmp.go:17:29: isResp escapes to heap

Oh, you're right. Thanks for correct～ @LENSHOOD

I found in this case that the escape analysis may be inaccurate.

tmp point$ go build -gcflags='-m' test.go
# command-line-arguments
./test.go:10:8: &bolt.Request{} does not escape
./test.go:16:28: ... argument escapes to heap
./test.go:16:58: isReq escapes to heap
tmp point$ cat test.go
package main

import (
	"mosn.io/mosn/pkg/log"
	"mosn.io/mosn/pkg/protocol/xprotocol/bolt"
)

func main() {
	var cmd interface{}
	cmd = &bolt.Request{}
	_, isReq := cmd.(*bolt.Request)
	_, isResp := cmd.(*bolt.Response)
	isStack := 0
	if log.DefaultLogger.GetLogLevel() >= log.DEBUG {
		if cmd != nil {
			log.DefaultLogger.Debugf("decode command, req(%v)\n", isReq)
		}
	}
	if false {
		if cmd != nil {
			log.DefaultLogger.Debugf("decode command, resp(%v)\n", isResp)
		}
	}
	println(&isReq)
	println(&isResp)
	println(&isStack)
}
tmp point$ go run test.go
0xc000145f67
0xc000145f66
0xc000145f68

I found a issue. golang/go#17725

I'm also in favor of deleting this judgment at the moment.
we can see if there are other scenarios besides @jizhuozhi case.

Hello, @diannaowa , @taoyuanyuan and @LENSHOOD .

Unfortunately, our previous inferences and reality may be somewhat different.

I'm currently working on a latency-based load balancer (#2252), and then doing related benchmarks and performance optimizations (since it's on the hotspot path). There is some logic of DEBUG level logs in my load balancer. In the flame graph, I found a convT64 function. However, there is no escape using pointers in my code, so the only possibility is that it is caused by logger. When I print the call stack, I find that it is true.

In fact, because the log accepts parameters are interface{} (or any in newest GO version)

Debugf(format string, args ...interface{})

so for the primitive type we pass in, even if we don't use the log, the conversion will still happen, which is really a big deal The overhead, so I think this change needs to be cautious, and I will continue to add judgment conditions here to avoid this situation from happening

This is somewhat different from Java. In Java, due to the existence of method inlining, under the action of the JIT compiler, the judgment can be promoted to the caller stack, thereby avoiding the automatic boxing problem caused by passing values. But this is not possible without PGO optimization in Go, so we are better off writing guard statements

Thank you for your kind reminder!
Indeed, we overlooked the issue of passing parameters as interface{}. Since there were no generics in Go before go1.18, for utils like Logger, we could only choose the interface{} type for the parameters.

To avoid dangling pointers, memory allocation requires that heap pointers must not point to the stack. Therefore, when the passed parameter is interface{}, for safety reasons, the compiler can only reallocate the passed parameter on the heap by using runtime.convT64(). This results in a heap allocation even when a primitive type is passed in.

Using generics MAY solve this problem (although I have not tested whether functions with varargs types will still be monomorphized based on GCShape).

Another way is to force the type to be determined at compile-time, similar to the approach used in Uber Zap:

logger.Info("failed to fetch URL",
  // Structured context as strongly typed Field values.
  zap.String("url", url),
  zap.Int("attempt", 3),
  zap.Duration("backoff", time.Second),
)

In short, any modification approach, whether using generics or the Uber Zap, may involve changes to hundreds of lines of code in MOSN. The evaluation of the actual performance impact may be considerable. Therefore, I also believe that simply removing the guard statements is too hasty. I agree with you to keep these guard statements and hope this issue can be resolved in the end.