Taking advantage of Time.AppendFormat #783
Comments
|
I'm not opposed to adding the That said, if we do support Since we can't extend the interface in 1.0, the included time encoders in zap could try and cast the passed in encoder to see if it supports an |
prashantv
added a commit
that referenced
this issue
Feb 13, 2020
Fixes #783. We can take advantage of Time.AppendFormat by adding a new exported method to the JSON encoder, and upcasting when encoding time to use the append-based method. This avoids an allocation to convert the time to a string before appending to the buffer. The benchmarks use the production config, which uses a nanos encoder so to understand the performance difference, I tweaked `BenchmarkZapJSON` to use RFC3339TimeEncoder and ran benchmarks: ``` > benchcmp -best old new benchmark old ns/op new ns/op delta BenchmarkZapJSON-12 514 497 -3.31% benchmark old allocs new allocs delta BenchmarkZapJSON-12 5 4 -20.00% benchmark old bytes new bytes delta BenchmarkZapJSON-12 1297 1265 -2.47% ``` I also wrote a benchmark that only logs a simple message using the RFC3339TimeEncoder, ``` func BenchmarkTimeEncoder(b *testing.B) { cfg := NewProductionConfig().EncoderConfig cfg.EncodeTime = zapcore.RFC3339TimeEncoder logger := New( zapcore.NewCore( zapcore.NewJSONEncoder(cfg), &ztest.Discarder{}, DebugLevel, )) b.ResetTimer() for i := 0; i < b.N; i++ { logger.Info("test") } } ``` Results: ``` > benchcmp -best old new benchmark old ns/op new ns/op delta BenchmarkTimeEncoder-12 695 571 -17.84% benchmark old allocs new allocs delta BenchmarkTimeEncoder-12 1 0 -100.00% benchmark old bytes new bytes delta BenchmarkTimeEncoder-12 32 0 -100.00% ```
prashantv
added a commit
that referenced
this issue
Feb 19, 2020
Fixes #783. We can take advantage of Time.AppendFormat by adding a new exported method to the JSON encoder, and upcasting when encoding time to use the append-based method. This avoids an allocation to convert the time to a string before appending to the buffer. The benchmarks use the production config, which uses a nanos encoder so to understand the performance difference, I tweaked `BenchmarkZapJSON` to use RFC3339TimeEncoder and ran benchmarks: ``` > benchcmp -best old new benchmark old ns/op new ns/op delta BenchmarkZapJSON-12 514 497 -3.31% benchmark old allocs new allocs delta BenchmarkZapJSON-12 5 4 -20.00% benchmark old bytes new bytes delta BenchmarkZapJSON-12 1297 1265 -2.47% ``` I also wrote a benchmark that only logs a simple message using the RFC3339TimeEncoder, ``` func BenchmarkTimeEncoder(b *testing.B) { cfg := NewProductionConfig().EncoderConfig cfg.EncodeTime = zapcore.RFC3339TimeEncoder logger := New( zapcore.NewCore( zapcore.NewJSONEncoder(cfg), &ztest.Discarder{}, DebugLevel, )) b.ResetTimer() for i := 0; i < b.N; i++ { logger.Info("test") } } ``` Results: ``` > benchcmp -best old new benchmark old ns/op new ns/op delta BenchmarkTimeEncoder-12 695 571 -17.84% benchmark old allocs new allocs delta BenchmarkTimeEncoder-12 1 0 -100.00% benchmark old bytes new bytes delta BenchmarkTimeEncoder-12 32 0 -100.00% ```
|
Just want to say thanks for working on this, much appreciated. |
cgxxv
pushed a commit
to cgxxv/zap
that referenced
this issue
Mar 25, 2022
Fixes uber-go#783. We can take advantage of Time.AppendFormat by adding a new exported method to the JSON encoder, and upcasting when encoding time to use the append-based method. This avoids an allocation to convert the time to a string before appending to the buffer. The benchmarks use the production config, which uses a nanos encoder so to understand the performance difference, I tweaked `BenchmarkZapJSON` to use RFC3339TimeEncoder and ran benchmarks: ``` > benchcmp -best old new benchmark old ns/op new ns/op delta BenchmarkZapJSON-12 514 497 -3.31% benchmark old allocs new allocs delta BenchmarkZapJSON-12 5 4 -20.00% benchmark old bytes new bytes delta BenchmarkZapJSON-12 1297 1265 -2.47% ``` I also wrote a benchmark that only logs a simple message using the RFC3339TimeEncoder, ``` func BenchmarkTimeEncoder(b *testing.B) { cfg := NewProductionConfig().EncoderConfig cfg.EncodeTime = zapcore.RFC3339TimeEncoder logger := New( zapcore.NewCore( zapcore.NewJSONEncoder(cfg), &ztest.Discarder{}, DebugLevel, )) b.ResetTimer() for i := 0; i < b.N; i++ { logger.Info("test") } } ``` Results: ``` > benchcmp -best old new benchmark old ns/op new ns/op delta BenchmarkTimeEncoder-12 695 571 -17.84% benchmark old allocs new allocs delta BenchmarkTimeEncoder-12 1 0 -100.00% benchmark old bytes new bytes delta BenchmarkTimeEncoder-12 32 0 -100.00% ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Hi,
I'm opening this issue to discuss a potential API change to the buffer type.
I have a custom encoder where times are encoded as strings formatted with the
Formatmethod from Go. Currently I have to do this:but it would be nice if we could just use the AppendFormat method to avoid an allocation.
I'm thinking of adding a method to the buffer type with this signature:
AppendTime(t time.Time, layout string).Another solution would be to allow users to set the byte slice used by the buffer type so that I could write this:
less elegant than having
AppendTimebut maybe a good solution if you don't want to special case times in the buffer type.The text was updated successfully, but these errors were encountered: