-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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
proposal: log/slog: copy Logger by value instead of pointer #59126
Comments
cc @jba |
Hmm, I thought I benchmarked this and the allocation was eliminated. I'll try to replicate your benchmarks. |
Regarding zero |
Oh, I see, the "With" benchmarks are your addition. I didn't care about
|
That depends on how users do contextual logging. One school of thought says that a logger should extract additional values from the context. Then the critical path only does The other school of thought (me including) points out that this only works in monolithic code bases where the logger configuration knows about all context keys. There also cannot be many of those, otherwise each log invocation would have to do many different IMHO it would be good if slog supported both approaches equally well. Note that a future |
@jba Code I've been reviewing the last few days using a different structured logging package has a flow like:
|
@ChrisHines, that code could easily be rewritten with an
|
@jba Yes, it could be written as you suggest, but in my experience it it tends to be less convenient and noisier code. Maybe you have some idea that I haven't thought of, could you show how you would write it? |
I agree. So do we have consensus that passing by value is the way to go? API design seems to allow both, and by value is more efficient. |
@ChrisHines how about
which should be much faster and only 1 extra line. I do think that Logger should pass by value, but I think that Logger.With is a much larger hammer that should only be used for cases where you are going to re-use the logger, not for single message construction. |
Of course that works @ianthehat , but the I think we should strive for much better, especially in the standard library. |
Making Yes, it's a bit faster if it's a value type. But more fundamentally, it must call So I don't see |
I feel that this particular code example is distracting from the underlying question: in cases where If the argument is that "Logger.With is slow anyway because the handler must do something", then let's look at a potential future |
Another argument against making Whether it's worth it just to speed up |
Ack.
That's the problem - estimating what "typical usage" is... By choosing pointer over value slog discourages contextual logging where a modified logger gets passed down a call chain. 🤷 |
Discouraging overuse of fluent APIs doesn't seem like a bad thing to me. I believe we should stick with the pointers. The potential future growth of the logger, as raised by @jba, is another important concern. When we needed to add monotonic time info to time.Time (a pure value struct) we had to work hard to keep it from growing. Also, the benchmark does not match real-world usage. In real usage, when a logger is passed to a small, analyzable routine, the compiler will see that it does not escape, and logger.With will be inlined, and the allocation will be made on the stack. The benchmark is going out of its way to avoid the stack allocation. And when the logger is passed to a large function, chances are that allocation is in the noise. |
A counter argument (that does not involve fluent APIs). |
I benchmarked Using a value |
This looks increasingly like what I'll propose for Kubernetes. A lot of code there is built around the
This only affects the "non-slog.Logger to slog.Logger" transition, but for that it is a valid point. The other API might construct a logger value and then doesn't need to allocate - logr does that. |
This proposal has been added to the active column of the proposals project |
time.Time was mentioned above. Speaking as someone who has had to extend time.Time over time and been quite constrained by needing to keep it small to make being a value reasonable, changing Logger to be a struct will limit us considerably in the future. It does not seem worth painting ourselves into that corner. |
I don't think it would paint you into a corner, if you really needed additional fields, you just make Logger point to an intermediate that held the handler and the extra fields, which would essentially put you right back where we are now in terms of allocations and cost. |
That would result in shared state on a copy, which might be wrong. |
My two cents: as seen in the proposal to add Having said that, I'm fine with whatever gets decided. |
Based on the discussion above, this proposal seems like a likely decline. |
No change in consensus, so declined. |
At the moment, https://pkg.go.dev/golang.org/x/exp/slog passes
Logger
around by pointer. The implication is that functions likeLogger.With
need one additional allocation, for the newLogger
instance.The alternative is to pass it as value, like go-logr does. This improves performance for
With
(see https://github.com/golang/exp/compare/master...pohly:exp:logger-as-value?expand=1) and others (not tested yet):The downside is that the size of
Logger
should not grow indefinitely in the future. But that is true anyway (because these calls must clone it) and also seems unlikely.Related to this is the question whether calling any of the methods for the null
Logger
should be valid. Currently they panic becauseLogger.handler
will be nil. I think this is the right choice given that they are performance-sensitive, but I wanted to ask anyway...The text was updated successfully, but these errors were encountered: