diff --git a/buffer/pool.go b/buffer/pool.go index 8fb3e202c..846323360 100644 --- a/buffer/pool.go +++ b/buffer/pool.go @@ -20,25 +20,29 @@ package buffer -import "sync" +import ( + "go.uber.org/zap/internal/pool" +) // A Pool is a type-safe wrapper around a sync.Pool. type Pool struct { - p *sync.Pool + p *pool.Pool[*Buffer] } // NewPool constructs a new Pool. func NewPool() Pool { - return Pool{p: &sync.Pool{ - New: func() interface{} { - return &Buffer{bs: make([]byte, 0, _size)} - }, - }} + return Pool{ + p: pool.New(func() *Buffer { + return &Buffer{ + bs: make([]byte, 0, _size), + } + }), + } } // Get retrieves a Buffer from the pool, creating one if necessary. func (p Pool) Get() *Buffer { - buf := p.p.Get().(*Buffer) + buf := p.p.Get() buf.Reset() buf.pool = p return buf diff --git a/error.go b/error.go index 65982a51e..38cb768de 100644 --- a/error.go +++ b/error.go @@ -21,14 +21,13 @@ package zap import ( - "sync" - + "go.uber.org/zap/internal/pool" "go.uber.org/zap/zapcore" ) -var _errArrayElemPool = sync.Pool{New: func() interface{} { +var _errArrayElemPool = pool.New(func() *errArrayElem { return &errArrayElem{} -}} +}) // Error is shorthand for the common idiom NamedError("error", err). func Error(err error) Field { @@ -60,7 +59,7 @@ func (errs errArray) MarshalLogArray(arr zapcore.ArrayEncoder) error { // potentially an "errorVerbose" attribute, we need to wrap it in a // type that implements LogObjectMarshaler. To prevent this from // allocating, pool the wrapper type. - elem := _errArrayElemPool.Get().(*errArrayElem) + elem := _errArrayElemPool.Get() elem.error = errs[i] arr.AppendObject(elem) elem.error = nil diff --git a/internal/pool/pool.go b/internal/pool/pool.go new file mode 100644 index 000000000..d1ac5588d --- /dev/null +++ b/internal/pool/pool.go @@ -0,0 +1,63 @@ +// Copyright (c) 2023 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package pool + +import ( + "sync" +) + +// A Constructor is a function that creates a T when a [Pool] is empty. +type Constructor[T any] func() T + +// A Pool is a generic wrapper around [sync.Pool] to provide strongly-typed +// object pooling. +type Pool[T any] struct { + pool sync.Pool + ctor Constructor[T] +} + +// New returns a new [Pool] for T, and will use ctor to construct new Ts when +// the pool is empty. +func New[T any](ctor Constructor[T]) *Pool[T] { + return &Pool[T]{ + pool: sync.Pool{ + New: func() any { + return ctor() + }, + }, + ctor: ctor, + } +} + +// Get gets a new T from the pool, or creates a new one if the pool is empty. +func (p *Pool[T]) Get() T { + if x, ok := p.pool.Get().(T); ok { + return x + } + + // n.b. This branch is effectively unreachable. + return p.ctor() +} + +// Put puts x into the pool. +func (p *Pool[T]) Put(x T) { + p.pool.Put(x) +} diff --git a/internal/pool/pool_norace_test.go b/internal/pool/pool_norace_test.go new file mode 100644 index 000000000..bd9db507e --- /dev/null +++ b/internal/pool/pool_norace_test.go @@ -0,0 +1,56 @@ +// Copyright (c) 2023 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +//go:build !race + +package pool_test + +import ( + "bytes" + "runtime/debug" + "testing" + + "github.com/stretchr/testify/require" + "go.uber.org/zap/internal/pool" +) + +func TestNew(t *testing.T) { + // n.b. Disable GC to avoid the victim cache during the test. + defer debug.SetGCPercent(debug.SetGCPercent(-1)) + + p := pool.New(func() *bytes.Buffer { + return bytes.NewBuffer([]byte("new")) + }) + p.Put(bytes.NewBuffer([]byte(t.Name()))) + + // Ensure that we always get the expected value. + for i := 0; i < 1_000; i++ { + func() { + buf := p.Get() + defer p.Put(buf) + require.Equal(t, t.Name(), buf.String()) + }() + } + + // Depool an extra object to ensure that the constructor is called and + // produces an expected value. + require.Equal(t, t.Name(), p.Get().String()) + require.Equal(t, "new", p.Get().String()) +} diff --git a/internal/pool/pool_race_test.go b/internal/pool/pool_race_test.go new file mode 100644 index 000000000..35971c8ba --- /dev/null +++ b/internal/pool/pool_race_test.go @@ -0,0 +1,69 @@ +// Copyright (c) 2023 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +//go:build race + +package pool_test + +import ( + "sync" + "testing" + + "go.uber.org/zap/internal/pool" +) + +type pooledValue struct { + n int64 +} + +func TestNew(t *testing.T) { + // n.b. [sync.Pool] will randomly drop re-pooled objects when race is + // enabled, so rather than testing nondeterminsitic behavior, we use + // this test solely to prove that there are no races. See pool_test.go + // for correctness testing. + + var ( + p = pool.New(func() *pooledValue { + return &pooledValue{ + n: -1, + } + }) + wg sync.WaitGroup + ) + + defer wg.Wait() + + for i := int64(0); i < 1_000; i++ { + i := i + + wg.Add(1) + go func() { + defer wg.Done() + + x := p.Get() + defer p.Put(x) + + // n.b. Must read and write the field. + if n := x.n; n >= -1 { + x.n = int64(i) + } + }() + } +} diff --git a/stacktrace.go b/stacktrace.go index 817a3bde8..1f152eb1a 100644 --- a/stacktrace.go +++ b/stacktrace.go @@ -22,19 +22,17 @@ package zap import ( "runtime" - "sync" "go.uber.org/zap/buffer" "go.uber.org/zap/internal/bufferpool" + "go.uber.org/zap/internal/pool" ) -var _stacktracePool = sync.Pool{ - New: func() interface{} { - return &stacktrace{ - storage: make([]uintptr, 64), - } - }, -} +var _stacktracePool = pool.New(func() *stacktrace { + return &stacktrace{ + storage: make([]uintptr, 64), + } +}) type stacktrace struct { pcs []uintptr // program counters; always a subslice of storage @@ -68,7 +66,7 @@ const ( // // The caller must call Free on the returned stacktrace after using it. func captureStacktrace(skip int, depth stacktraceDepth) *stacktrace { - stack := _stacktracePool.Get().(*stacktrace) + stack := _stacktracePool.Get() switch depth { case stacktraceFirst: diff --git a/zapcore/console_encoder.go b/zapcore/console_encoder.go index 1aa5dc364..8ca0bfaf5 100644 --- a/zapcore/console_encoder.go +++ b/zapcore/console_encoder.go @@ -22,20 +22,20 @@ package zapcore import ( "fmt" - "sync" "go.uber.org/zap/buffer" "go.uber.org/zap/internal/bufferpool" + "go.uber.org/zap/internal/pool" ) -var _sliceEncoderPool = sync.Pool{ - New: func() interface{} { - return &sliceArrayEncoder{elems: make([]interface{}, 0, 2)} - }, -} +var _sliceEncoderPool = pool.New(func() *sliceArrayEncoder { + return &sliceArrayEncoder{ + elems: make([]interface{}, 0, 2), + } +}) func getSliceEncoder() *sliceArrayEncoder { - return _sliceEncoderPool.Get().(*sliceArrayEncoder) + return _sliceEncoderPool.Get() } func putSliceEncoder(e *sliceArrayEncoder) { diff --git a/zapcore/entry.go b/zapcore/entry.go index 9d326e95e..059844f92 100644 --- a/zapcore/entry.go +++ b/zapcore/entry.go @@ -24,25 +24,23 @@ import ( "fmt" "runtime" "strings" - "sync" "time" "go.uber.org/multierr" "go.uber.org/zap/internal/bufferpool" "go.uber.org/zap/internal/exit" + "go.uber.org/zap/internal/pool" ) -var ( - _cePool = sync.Pool{New: func() interface{} { - // Pre-allocate some space for cores. - return &CheckedEntry{ - cores: make([]Core, 4), - } - }} -) +var _cePool = pool.New(func() *CheckedEntry { + // Pre-allocate some space for cores. + return &CheckedEntry{ + cores: make([]Core, 4), + } +}) func getCheckedEntry() *CheckedEntry { - ce := _cePool.Get().(*CheckedEntry) + ce := _cePool.Get() ce.reset() return ce } diff --git a/zapcore/error.go b/zapcore/error.go index 06359907a..c67dd71df 100644 --- a/zapcore/error.go +++ b/zapcore/error.go @@ -23,7 +23,8 @@ package zapcore import ( "fmt" "reflect" - "sync" + + "go.uber.org/zap/internal/pool" ) // Encodes the given error into fields of an object. A field with the given @@ -103,9 +104,9 @@ func (errs errArray) MarshalLogArray(arr ArrayEncoder) error { return nil } -var _errArrayElemPool = sync.Pool{New: func() interface{} { +var _errArrayElemPool = pool.New(func() *errArrayElem { return &errArrayElem{} -}} +}) // Encodes any error into a {"error": ...} re-using the same errors logic. // @@ -113,7 +114,7 @@ var _errArrayElemPool = sync.Pool{New: func() interface{} { type errArrayElem struct{ err error } func newErrArrayElem(err error) *errArrayElem { - e := _errArrayElemPool.Get().(*errArrayElem) + e := _errArrayElemPool.Get() e.err = err return e } diff --git a/zapcore/json_encoder.go b/zapcore/json_encoder.go index de9e2c162..ce6838de2 100644 --- a/zapcore/json_encoder.go +++ b/zapcore/json_encoder.go @@ -23,24 +23,20 @@ package zapcore import ( "encoding/base64" "math" - "sync" "time" "unicode/utf8" "go.uber.org/zap/buffer" "go.uber.org/zap/internal/bufferpool" + "go.uber.org/zap/internal/pool" ) // For JSON-escaping; see jsonEncoder.safeAddString below. const _hex = "0123456789abcdef" -var _jsonPool = sync.Pool{New: func() interface{} { +var _jsonPool = pool.New(func() *jsonEncoder { return &jsonEncoder{} -}} - -func getJSONEncoder() *jsonEncoder { - return _jsonPool.Get().(*jsonEncoder) -} +}) func putJSONEncoder(enc *jsonEncoder) { if enc.reflectBuf != nil { @@ -354,7 +350,7 @@ func (enc *jsonEncoder) Clone() Encoder { } func (enc *jsonEncoder) clone() *jsonEncoder { - clone := getJSONEncoder() + clone := _jsonPool.Get() clone.EncoderConfig = enc.EncoderConfig clone.spaced = enc.spaced clone.openNamespaces = enc.openNamespaces diff --git a/zapgrpc/internal/test/go.sum b/zapgrpc/internal/test/go.sum index b2a26825e..51be79372 100644 --- a/zapgrpc/internal/test/go.sum +++ b/zapgrpc/internal/test/go.sum @@ -53,8 +53,6 @@ github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= -github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= -github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA=