Skip to content
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

Open
ianlancetaylor opened this issue Jan 1, 2025 · 24 comments
Open

proposal: sync/v2: new package #71076

ianlancetaylor opened this issue Jan 1, 2025 · 24 comments
Labels
Proposal v2 An incompatible library change
Milestone

Comments

@ianlancetaylor
Copy link
Member

ianlancetaylor commented Jan 1, 2025

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:

func OnceFunc(f func()) func()
func OnceValue[T any](f func() T) func() T
func OnceValues[T1, T2 any](f func() (T1, T2)) func() (T1, T2)
type Cond struct{ ... }
    func NewCond(l Locker) *Cond
type Locker interface{ ... }
type Mutex struct{ ... }
type Once struct{ ... }
type RWMutex struct{ ... }
type WaitGroup struct{ ... }

The existing Map type will be replaced by a new type that takes two type parameters. Note that the original Range method is replaced with an All method that returns an iterator.

// Map is like a Go map[K]V but is safe for concurrent use
// by multiple goroutines without additional locking or coordination.
// ...and so forth
type Map[K comparable, V any] struct { ... }

// Load returns the value stored in the map for a key, or the zero value if no
// value is present.
// The ok result indicates whether value was found in the map.
func (m *Map[K, V]) Load(key K) (value V, ok bool)

// Store sets the value for a key.
func (m *Map[K, V]) Store(key K, value V)

// LoadOrStore returns the existing value for the key if present.
// Otherwise, it stores and returns the given value.
// The loaded result is true if the value was loaded, false if stored.
func (m *Map[K, V]) LoadOrStore(key K, value V) (actual V, loaded bool)

// LoadAndDelete deletes the value for a key, returning the previous value if any.
// The loaded result reports whether the key was present.
func (m *Map[K, V]) LoadAndDelete(key K) (value V, loaded bool)

// Delete deletes the value for a key.
func (m *Map[K, V]) Delete(key K)

// Swap swaps the value for a key and returns the previous value if any.
// The loaded result reports whether the key was present.
func (m *Map[K, V]) Swap(key K, value V) (previous V, loaded bool)

// CompareAndDelete deletes the entry for key if its value is equal to old.
// This panics if V is not a comparable type.
//
// If there is no current value for key in the map, CompareAndDelete
// returns false.
func (m *Map[K, V]) CompareAndDelete(key K, old V) (deleted bool)

// CompareAndSwap swaps the old and new values for key
// if the value stored in the map is equal to old.
// This panics if V is not a comparable type.
func (m *Map[K, V]) CompareAndSwap(key K, old, new V) (swapped bool)

// Clear deletes all the entries, resulting in an empty Map.
func (m *Map[K, V]) Clear()

// All returns an iterator over the keys and values in the map.
// ... and so forth
func (m *Map[K, V]) All() iter.Seq2[K, V]

// TODO: Consider Keys and Values methods that return iterators, like maps.Keys and maps.Values.

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 exported New field; instead, use NewPool to create a Pool that calls a function to return new values.

Note that the original Get method is replaced by one that returns two results, with the second being a bool 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 of T.

// A PoolOf is a set of temporary objects of type T that may be individually saved and retrieved.
// ...and so forth
type Pool[T any] struct {
    ...
}

// 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.
func NewPool[T any] func(newf func() T) *Pool[T]

// Put adds x to the pool.
func (p *Pool[T]) Put(x T)

// Get selects an arbitrary item from the Pool, removes it from the
// Pool, and returns it to the caller.
// ...and so forth
//
// If Get does not have a value to return, and p was created with a call to [NewPool] with a non-nil argument,
// Get returns the result of calling the function passed to [NewPool].
func (p *Pool[T]) Get() T
@ianlancetaylor ianlancetaylor added this to the Proposal milestone Jan 1, 2025
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Jan 1, 2025
@gabyhelp
Copy link

gabyhelp commented Jan 1, 2025

@mateusz834
Copy link
Member

There is one issue I’ve encountered with the current sync.Pool is that the New func() field is exported. This makes it easy to accidentally call New directly instead of using the Get method. Maybe we want to hide it somehow?

How the Pool is going to behave with non-pointers? Will Put() cause allocation for let's say []byte, instead of *[]byte

@Jorropo
Copy link
Member

Jorropo commented Jan 1, 2025

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 Mutex),
it also allows to remove 2 pointers from Cond,
and makes it easier for the compiler to remove a virtual dispatch inside Cond.

Other part of me thinks this is too much #darkarts,
is not easily retro compatible with the old Locker interface approach,
and looks a lot like the painful parts of rust.

@Merovius
Copy link
Contributor

Merovius commented Jan 1, 2025

I'm wondering if there's even a need to make Cond polymorphic. Are there any sync.Locker implementations out there that are not just *Mutex or *RWMutex? That is, would it perhaps be better to just make it Cond struct{ L Mutex }, or have two types, or make it Cond[Locker Mutex|RWMutex] struct{ L Locker }?

I'm not familiar with the use cases for Cond (I have literally never used it). But part of that is the necessity of providing a Locker explicitly, which confused me as to how it was intended to be used.

@thepudds
Copy link
Contributor

thepudds commented Jan 1, 2025

I don't fully understand this piece:

type Map[K comparable, V any] struct { ... }

[...]

// CompareAndDelete deletes the entry for key if its value is equal to old.
// This panics if V is not a comparable type.
//
// If there is no current value for key in the map, CompareAndDelete
// returns false.
func (m *Map[K, V]) CompareAndDelete(key K, old V) (deleted bool)```

For panicking if V is not a comparable type and doing the equality check if V is a comparable type —
is that implementable in ordinary Go code in the current version of the compiler without resorting to reflect?

Is the intent of this proposal to use reflect, or some stdlib-only compiler or runtime magic, or would this be contingent on #45380 or some other generics enhancement?

In #47657 (comment), Ian had written:

One possibility is to permit generic types to have methods that use different, tighter, constraints. Those methods would only be available if the type argument(s) satisfied those constraints. I don't know whether that would be a good idea or not.

Separately, @Merovius pointed out in #47657 (comment):

I'll note that if we adopted #65394, you could have the constraints on Map be [K comparable, V any] and the constraints on CompareAndSwap be [K, V comparable] - that is, you would get those methods if and only if the value type is comparable as well. Having type-safety and flexibility.

... but I'm not sure what solution this proposal is relying upon (or maybe I misunderstood the issue 😅).

@Merovius
Copy link
Contributor

Merovius commented Jan 1, 2025

is that implementable in ordinary Go code in the current version of the compiler without resorting to reflect?

Just realized: A trivial way to get those semantics is any(v1) == any(v2). But still, I'd strongly prefer a type-safe solution.

@jub0bs
Copy link

jub0bs commented Jan 1, 2025

This is exciting!

Should Cond even be retained, though? @bcmills cogently lobbies for its deprecation. I'd rather see the inclusion of something like https://github.com/neild/gate.

@ianlancetaylor
Copy link
Member Author

@mateusz834

There is one issue I’ve encountered with the current sync.Pool is that the New func() field is exported. This makes it easy to accidentally call New directly instead of using the Get method. Maybe we want to hide it somehow?

I think the goals here include

  • zero value of sync.Pool should be useful
  • some sort of "new" function is desirable for pooled types that requires initialization
  • it should be easy to use sync.Pool as a package-scope variable

When those goals are put together, it seems hard to avoid having sync.Pool be a struct with an exported New field.

I am definitely open to suggestions.

How the Pool is going to behave with non-pointers? Will Put() cause allocation for let's say []byte, instead of *[]byte

I'm not specifying an implementation, but looking at the current implementation it seems straightforward for the v2 sync.Pool to use values of type T rather than type any, so I don't see why a Pool[[]byte] would allocate on Put. The main change is that we'll need to store an additional array of bool values in the deque to indicate whether a slot holds a valid value—the current code can use nil for an invalid value.

@ianlancetaylor
Copy link
Member Author

I think we could in principle drop Cond from sync/v2 if we made it available in some other package, perhaps in x/sync. I don't know whether that would be a good idea or not. I note that the current implementation of sync.Cond relies on runtime package support.

I think that in practical use a Cond should not incorporate a Locker, it should only refer to one. Condition variables are only useful in conjunction with some sort of mutex, but code will often want to use that mutex in other ways. For example, see connReader in the net/http package. It has a cond field that uses the mu field, but the code also uses the mu field in various ways that don't touch the cond field at all. Overloading the mutex into the condition variable will make this code more obscure.

If we were designing from scratch it might make sense to make the Cond simply use a Mutex, but it would not be surprising if there are Cond variables out there that use a RWMutex, and it seems unnecessary to break them.

@seankhliao
Copy link
Member

since the original sync package will continue to be available with Cond, do we really need to have another copy elsewhere?

@ianlancetaylor
Copy link
Member Author

@thepudds What I'm trying to express in CompareAndDelete and CompareAndSwap is isomorphic to the current documentation, which says "The old value must be of a comparable type." And as @Merovius says we can use any(v1) == any(v2), which will panic if the values are not comparable. It's pretty horrible but I don't see another way to preserve the current semantics.

@neild
Copy link
Contributor

neild commented Jan 1, 2025

When those goals are put together, it seems hard to avoid having sync.Pool be a struct with an exported New field.

We could have a NewPool function that initializes a pool with a constructor function.

// NewPool returns a Pool that uses newf to construct new objects.
func NewPool[T any](newf func() T) *Pool[T]
  • Zero value Pool can behave as today.
  • No harder to construct a pool with a New function than it is now.
  • Package-scope variable initialization works. (var pool = NewPool(func() T { ... }))
  • Can't accidentally call Pool.New instead of Pool.Get.
  • Can't change the New function after pool construction, but that seems like a benefit rather than a limitation.

@ianlancetaylor
Copy link
Member Author

@seankhliao

since the original sync package will continue to be available with Cond, do we really need to have another copy elsewhere?

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.

@neild
Copy link
Contributor

neild commented Jan 1, 2025

I don't think we should change Pool.Get to return an additional bool.

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 Get to report whether it found an object. It can return the zero value if nothing is found, and the caller can check for the zero value if they care. Most callers won't care, and can use the result of Get directly.

@jub0bs
Copy link

jub0bs commented Jan 1, 2025

@neild Incidentally, wouldn't the advent of v2 be a good opportunity to enforce at compile time that pointers be used?

type Pool[T any] struct {
    // contains filtered or unexported fields
}

func NewPool[T any](newf func() *T) *Pool[T]

func (p *Pool[T]) Put(x *T)

func (p *Pool[T]) Get() *T

Edit: just ignore me; I had overlooked #71076 (comment).

@DmitriyMV
Copy link
Contributor

@jub0bs this will force people to store pointers to the slices which was quite a big problem with the original Pool.

@dsnet
Copy link
Member

dsnet commented Jan 2, 2025

@neild's comment echos prior comments, which seems to be supported by various others.

As some empirical experience supporting dropping ok from sync.Pool.Get, we have syncs.Pool at Tailscale, which is a generic wrapper over the v1 sync.Pool. Our implementation drops ok, and we have not missed having it in our various usages throughout our codebase.

@mateusz834
Copy link
Member

mateusz834 commented Jan 2, 2025

Incidentally, wouldn't the advent of v2 be a good opportunity to enforce at compile time that pointers be used?

What about pooling maps? They are internally pointers, now we would have unnecessary allocations?

@jub0bs
Copy link

jub0bs commented Jan 2, 2025

Before considering folding golang.org/x/sync/errgroup into sync/v2, its API should be improved. Relevant issue (and comments): #57534.

@earthboundkid
Copy link
Contributor

Could it be type Pool[P ~[]T | ~*T, T any] then? Maps are an issue, but I don't see the harm in those being pointed to instead of direct, since it's a fairly uncommon need.

I'm in favor of dropping Cond fwiw. It's so misunderstood, the documentation for context.AfterFunc originally misused it.

@earthboundkid
Copy link
Contributor

Could it be type Pool[P ~[]T | ~*T, T any] then? Maps are an issue, but I don't see the harm in those being pointed to instead of direct, since it's a fairly uncommon need.

Playing with it on the playground, this is legal syntax, but the type inference requires both type parameters, which is unfortunate.

@Merovius
Copy link
Contributor

Merovius commented Jan 2, 2025

What exactly is the issue with just making it type Pool[T any]? What's the failure mode we are trying to address? It seems to me, at worst it's doing nothing. And presumably, if you are trying to use a sync.Pool, you do so to fix a problem and you'll figure out pretty quickly that it doesn't actually get fixed.

I also don't think there is a reason to forbid struct{ x A; b *B } from being put into a pool, to save allocations of Bs.

Restricting that does, IMO, nothing to help, but is unnecessarily restrictive.

@DmitriyMV
Copy link
Contributor

Could it be type Pool[P ~[]T | ~*T, T any] then?

That would prevent reusing hash.Hash instances, which is expected in cases where you reach for sync.Pool.

Maps are an issue, but I don't see the harm in those being pointed to instead of direct, since it's a fairly uncommon need.

There are other types which have pointers inside.

@ianlancetaylor
Copy link
Member Author

@neild Thanks, I've updated the proposal to remove the New field of Pool, to add NewPool, and to change Get to just return T.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal v2 An incompatible library change
Projects
Status: Incoming
Development

No branches or pull requests