-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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: sync/v2: new package #71076
Comments
Related Issues
Related Documentation Related Discussions (Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.) |
There is one issue I’ve encountered with the current How the |
Part of me wants to change to: type CondLocker[T any] interface {
Locker
*T
}
type Cond[T any, L CondLocker[T]] struct {
L T
}
// Random example pseudo code to prove this is exploitable
func (c *Cond[T, L]) Test() {
var p L = &c.L
p.Lock()
p.Unlock()
}
// ... this makes the zero value of a cond valid if the locker's zero value is valid (like Other part of me thinks this is too much |
I'm wondering if there's even a need to make I'm not familiar with the use cases for |
I don't fully understand this piece:
For panicking if V is not a comparable type and doing the equality check if V is a comparable type — Is the intent of this proposal to use In #47657 (comment), Ian had written:
Separately, @Merovius pointed out in #47657 (comment):
... but I'm not sure what solution this proposal is relying upon (or maybe I misunderstood the issue 😅). |
Just realized: A trivial way to get those semantics is |
This is exciting! Should |
I think the goals here include
When those goals are put together, it seems hard to avoid having I am definitely open to suggestions.
I'm not specifying an implementation, but looking at the current implementation it seems straightforward for the v2 |
I think we could in principle drop I think that in practical use a If we were designing from scratch it might make sense to make the |
since the original sync package will continue to be available with Cond, do we really need to have another copy elsewhere? |
@thepudds What I'm trying to express in |
We could have a NewPool function that initializes a pool with a constructor function.
|
I think a goal here should be that people can, with a little bit of work, change from using sync to using sync/v2. We don't want a state in which people are using both the sync and sync/v2 package with no prospect of ever moving completely to the sync/v2 package. |
I don't think we should change A Pool must return a pointer or pointer-containing object to be useful. (A Pool is a pool of references to temporary objects. Since a Pool doesn't report when objects are evicted, those references must be garbage collected, and therefore must be pointers.) If the pooled objects contain pointers, it is possible to distinguish between the zero value reference and an initialized reference. Therefore, there is no need for |
This comment has been minimized.
This comment has been minimized.
@jub0bs this will force people to store pointers to the slices which was quite a big problem with the original Pool. |
@neild's comment echos prior comments, which seems to be supported by various others. As some empirical experience supporting dropping |
What about pooling |
Before considering folding golang.org/x/sync/errgroup into sync/v2, its API should be improved. Relevant issue (and comments): #57534. |
Could it be I'm in favor of dropping |
Playing with it on the playground, this is legal syntax, but the type inference requires both type parameters, which is unfortunate. |
What exactly is the issue with just making it I also don't think there is a reason to forbid Restricting that does, IMO, nothing to help, but is unnecessarily restrictive. |
That would prevent reusing
There are other types which have pointers inside. |
@neild Thanks, I've updated the proposal to remove the |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Will it use type aliases for |
That seems like a good idea, there are no API changes for: type Cond struct{ ... }
func NewCond(l Locker) *Cond
type Locker interface{ ... }
type Mutex struct{ ... }
type Once struct{ ... }
type RWMutex struct{ ... }
type WaitGroup struct{ ... } So it would be nice to alias these types in |
It's not clear from my reading so far why this is v2 instead of just adding |
@earthboundkid #47657 (comment) You're right, of course, that we could use |
If v2 is on the table, I don't think it should shirk from distancing itself from v1's API or functionalities. |
I don't think removing
There's lots of code which uses condition variables. Unless we decide we should encourage it all to be rewritten (which I do not think we should do), we're going to have Cond. |
I'd like to suggest a new method func (wg *WaitGroup) Notify() <- chan struct{} The primary goal would be to make it slightly easier to handle context cancelation while waiting for a Here's a common pattern in many codebases: c := make(chan struct{})
go func() {
wg.Wait()
close(c)
}()
select {
case <-ctx.Done():
return context.Cause(ctx)
case <-c:
return nil
} With this new method: select {
case <- ctx.Done():
return context.Cause(ctx)
case <- wg.Notify():
return nil
} Edit: Fixed pointer receiver. |
@mohamedattahri Thanks, that's an interesting idea. It should be a separate proposal, though. We could do that entirely independently of creating a sync/v2 package. |
This comment has been minimized.
This comment has been minimized.
Before we commit to: func NewPool[T any] func(newf func() T) *Pool[T] I want to point out a common use-case I see across multiple codebases, which is the ability to cleanup the value before putting it back in the pool. Today, due to the lack of generics, authors may reasonably add two helper functions like this: var decoderPool = sync.Pool{
New: func() any { return new(Decoder) },
}
func getDecoder() *Decoder {
return decoderPool.Get().(*Decoder)
}
func putDecoder(d *Decoder) {
d.reader = nil // avoid pinning the reader before placing in pool
decoderPool.Put(d)
} A generic version of However, we still can't quite get rid of I propose that // NewPool returns a new pool.
//
// If the newf argument is not nil, then when the pool is empty,
// newf is called to fetch a new value.
// This is useful when the values in the pool should be initialized.
//
// If the clearf argument is not nil, then it is called on each value
// just prior to putting back into the pool.
// If clearf returns false, the value is dropped from the pool.
// This is useful to enforce any form of cleanup on values
// such as clearing out pointers to other memory that is not
// intended to be part of the pooled value or
// to avoid pooling a value that has become too costly to retain.
func NewPool[T any] func(newf func() T, clearf func(T) bool) *Pool[T] The use case of a "discard" return bool is for cases like: var bufferPool = sync.New(nil, func(b []byte) bool {
return cap(b) < 1<<16 // avoid pinning excessively large buffers
}) |
@dsnet Shouldn't the clearf take a pointer? Let's say you |
Maybe the
|
Instead of separate new / clear, what about a single reset func like bytes.Buffer.Reset has which should be valid for preparing a newly allocated zero value or resetting a previously used one? |
I'm not sure how we can combine |
Possibly, but I suspect that this will have performance problems since An alternative API is If we had something like #26842, we could simplify this to just I should note that
|
BTW, should #26842 be reopen? |
A common case for clearf is to reset a |
The existing I think it's worth having a similar convenience for users who need a custom recycle function. It's unlikely that we'll add additional methods to I propose (this matches #71076 (comment)):
Usage:
I think that should handle any case where someone might otherwise feel the need to wrap a pool. The constructor makes pool initialization a bit less nice, but I don't think it's all that bad and pools aren't constructed all that often anyway. Alternatively, we could keep the v1 design of having newf/recyclef be fields of Pool, but give those fields names that are less likely to be accidentally called: |
Proposal Details
The math/rand/v2 package has been successful. Let's consider another v2 package: sync/v2.
This is an update of #47657.
In the sync/v2 package, the following types and functions will be the same as in the current sync package:
The existing
Map
type will be replaced by a new type that takes two type parameters. Note that the originalRange
method is replaced with anAll
method that returns an iterator.The existing
Pool
type will be replaced by a new type that takes a type parameter.2025-01-04: The new version of
Pool
does not have an exportedNew
field; instead, useNewPool
to create aPool
that calls a function to return new values.Note that the originalGet
method is replaced by one that returns two results, with the second being abool
indicating whether a value was returned. This is only useful if the New field is optional: we could also change this to require the New field to be set, or to provide a default implementation that returns the zero value ofT
.The text was updated successfully, but these errors were encountered: