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

sync: add OnceFunc, OnceValue, OnceValues #56102

Closed
adg opened this issue Oct 7, 2022 · 115 comments
Closed

sync: add OnceFunc, OnceValue, OnceValues #56102

adg opened this issue Oct 7, 2022 · 115 comments
Labels
Milestone

Comments

@adg
Copy link
Contributor

adg commented Oct 7, 2022

This is a proposal for adding a generic OnceFunc function to the sync package in the standard library.

In my team's codebase we recently added this function to our private syncutil package:

// OnceFunc returns a function that invokes fn only once and returns the values
// returned by fn. The returned function may be called concurrently.
func OnceFunc[T any](fn func() (T, error)) func() (T, error)

(I put this in the (temporary) module github.com/adg/sync, if you want to try it out.)

This makes a common use of sync.Once, lazy initialization with error handling, more ergonomic.

For example, this Server struct that wants to lazily initialize its database connection may use sync.Once:

type Server struct {
	dbPath string

	dbInit sync.Once
	dbVal  *sql.DB
	dbErr  error
}

func NewServer(dbPath string) *Server {
	return &Server{
		dbPath: dbPath,
	}
}

func (s *Server) db() (*sql.DB, error) {
	s.dbInit.Do(func() {
		s.dbVal, s.dbErr = sql.Open("sqlite", s.dbPath)
	})
	return s.dbVal, s.dbErr
}

func (s *Server) DoSomething() error {
	db, err := s.db()
	if err != nil {
		return err
	}
	_ = db // do something with db
	return nil
}

While with OnceFunc a lot of the fuss goes away:

type Server struct {
	db func() (*sql.DB, error)
}

func NewServer(dbPath string) *Server {
	return &Server{
		db: sync.OnceFunc(func() (*sql.DB, error) {
			return sql.Open("sqlite", dbPath)
		}),
	}
}

func (s *Server) DoSomething() error {
	db, err := s.db()
	if err != nil {
		return err
	}
	_ = db // do something with db
	return nil
}

Playground links: before and after.

If there is interest in this, then I suppose it should first live in x/exp (as with the slices and maps packages) so that we can play with it.

This seems to me like a great example of how generics can be used in the standard library. I wasn't able to find an overall tracking bug for putting generics in the standard library, otherwise I'd have referenced it here.

@adg adg added Proposal generics Issue is related to generics labels Oct 7, 2022
@gopherbot gopherbot added this to the Proposal milestone Oct 7, 2022
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Oct 8, 2022
@icholy
Copy link

icholy commented Oct 8, 2022

Alternative API:

type Server struct {
	once sync.TryOnce[*sql.DB]
}

func (s *Server) db() (*sql.DB, error) {
	return s.once.Do(func() (*sql.DB, error) {
		return sql.Open("sqlite", dbPath)
	})
}

@earthboundkid
Copy link
Contributor

I have one of these in https://github.com/carlmjohnson/syncx. I think it’s suitable for the standard library, but it should probably come as part of a std wide addition of generics, not a one off.

@cespare
Copy link
Contributor

cespare commented Oct 8, 2022

I played around with this in a large codebase. It's a nice idea.

I do prefer @icholy's API suggestion. It seems that the ergonomics are about the same and also that it would be a little harder to accidentally misuse. I could see people writing

func GetFoo() (*Foo, error) {
        return OnceFunc(getFoo)
}

whereas the equivalent mistake with TryOnce seems less likely (especially if people are used to sync.Once already). And in general, it just seems little nicer/more typical to have the shared state be a struct value rather than a function; for example, consider

var fooOnce TryOnce[*Foo]

func GetFoo() (*Foo, error) { return fooOnce.Do(getFoo) }

vs.

var getFooOnce = OnceFunc(getFoo)

func GetFoo() (*Foo, error) { return getFooOnce() }

When I was looking at how sync.Once gets used in my codebase, I found that I could categorize them roughly four ways:

  1. A single return value (either no possibility of an error or else the error leads to os.Exit, log.Fatal, etc).
  2. Two return values: (T, error).
  3. No initialization values created (just a one-time check or something)
  4. Multiple values initialized / a bunch of state configured

For (3) and (4), sync.Once seems about optimal right now. This proposal helps with (2). But I noticed that (1) is even more common than (2). So maybe having both would be best:

type OnceVal[T any] struct { /* ... */ }

func (o *OnceVal[T]) Do(f func() T) T

type OnceError[T any] struct { /* ... */ }

func (o *OnceError[T]) Do(f func() (T, error)) (T, error)

or even

type Once1[T any] struct { /* ... */ }

func (o *Once1[T]) Do(f func() T) T

type Once2[T1, T2 any] struct { /* ... */ }

func (o *Once2[T1, T2]) Do(f func() (T1, T2)) (T1, T2)

OneOfOne added a commit to OneOfOne/genh that referenced this issue Oct 8, 2022
@earthboundkid
Copy link
Contributor

earthboundkid commented Oct 9, 2022

FWIW, I wrote the closure version and find it much more ergonomic. It wouldn't occur to me to write

func GetFoo() (*Foo, error) {
        return OnceFunc(getFoo)
}

since it obviously should be var GetFoo = sync.OnceFunc(getFoo), but it's hard to predict what kind of error people will make en mass until it's in the wild.

@earthboundkid
Copy link
Contributor

Also I don't think it makes sense for this to return an error for the reasons given here: #53696 (comment)

I also had a situation when I wanted to make some initialization in my app lazy (because it runs on Lambda and cold start is a pain) but the initialization could potentially fail, but once that's the situation, there's no good API (at least that I've seen). The errors have to be dealt with somewhere. (If they could be ignored, regular sync.Once would work.) If the system assumes initialization has already happened, the path to deal with the error isn't there and all you can do is crash. If it doesn't make that assumption, you need to handle the error every time you interact with the object, so it's not really "initialized" just "gettable".

@adg
Copy link
Contributor Author

adg commented Oct 9, 2022

@carlmjohnson wrote:

The errors have to be dealt with somewhere. (If they could be ignored, regular sync.Once would work.) If the system assumes initialization has already happened, the path to deal with the error isn't there and all you can do is crash. If it doesn't make that assumption, you need to handle the error every time you interact with the object, so it's not really "initialized" just "gettable".

I proposed a OnceFunc that returns (T, error) because my code, and other code I have observed in the wild, often stores an initialization error alongside the initialized value.

A few examples I quickly pulled from the Go core (there are many more, I didn't want to look exhaustively):

Your circumstances may call for a different error handling mechanism (crashing), but others prefer to handle the error every time the resource is requested. Then upstream callers can decide whether it's crash-worthy or not.

I think that without the error return value the proposed OnceFunc is not very useful. Otherwise you should just do the initialization earlier, since the program should crash if the resource isn't available anyway, or you don't care about handling the error in which case (as you say) the existing sync.Once gives you almost everything you need already.

@adg
Copy link
Contributor Author

adg commented Oct 9, 2022

@icholy suggested:

Alternative API:

Let me just expand your suggested API to do exactly what the other examples are doing, so that it's a fair comparison:

type Server struct {
        dbPath string
        dbOnce sync.TryOnce[*sql.DB]
}

func NewServer(dbPath string) *Server {
        return &Server{dbPath: dbPath}
}

func (s *Server) db() (*sql.DB, error) {
        return s.dbOnce.Do(func() (*sql.DB, error) {
                return sql.Open("sqlite", s.dbPath)
        })
}

func (s *Server) DoSomething() error {
        db, err := s.db()
        ...
}

I like that the type has a usable zero value, which means you don't need a constructor for Server just to set up this value (but we do need the something to set the dbPath field, or whatever other state goes into the closure). However in exchange for that we need a wrapper function (the db method here), so we immediately return to equal in terms of boilerplate.

I like that baking the once-ness into the type declaration, instead of just using a plain closure, gives some indication on sight that it's a once-initialized value.

Here's a variation on what you suggest, which is arguably less boilerplatey, as we don't need to store the closure state anywhere. TheNewOnceFunc function returns a *OnceFunc[T] with a Do() (T, error) method:

type Server struct {
	db *sync.OnceFunc[*sql.DB]
}

func NewServer(dbPath string) *Server {
	return &Server{
		db: sync.NewOnceFunc(func() (*sql.DB, error) {
			return sql.Open("sqlite", dbPath)
		}),
	}
}

func (s *Server) DoSomething() error {
	db, err := s.db.Do()
	...
}

But to immediately argue against this: an advantage of baking the state (dbPath, in in this example) into the once function itself is that we don't expect that changing it later will have any effect. For instance, if we changed dbPath after the first call to the db function we might expect to access a different database. Putting that state in the closure makes it harder to make that mistake.

With all that said, my main objection to these proposals (compared to my original proposal) is that they make it harder to substitute a different initialization function that doesn't use OnceFunc. A central advantage of my original proposed API is that a sync.OnceFunc can wrap any func() (T, error) transparently, so that downstream callers don't know (and shouldn't care) that they're invoking it only once. In my experience this is a valuable property.

@adg
Copy link
Contributor Author

adg commented Oct 9, 2022

@cespare my instinct is that having fewer things is better than more things. If someone wants to call a OnceFunc and ignore the error, they could just ignore the error.

@cespare
Copy link
Contributor

cespare commented Oct 10, 2022

However in exchange for that we need a wrapper function (the db method here)

I do think that a db method is nicer than a function as a struct field. That strikes me as unusual-looking about your original example.

Also, the original example doesn't look like the common use cases I see for sync.Once. I most often see sync.Once used for package-level initialization. That's what most of the links you located in #56102 (comment) are as well.

So we have something like this today:

var state struct {
	once sync.Once
	val *Thing
	err error
}

func State() (*Thing, error) {
	state.once.Do(func() {
		state.val, state.err = loadState()
	})
	return state.val, state.err
}

func loadState() (*Thing, error) { /* ... */ }

With OnceFunc, it could be

var loadStateOnce = sync.OnceFunc(loadState)

func State() (*Thing, error) {
	return loadStateOnce()
}

func loadState() (*Thing, error) { /* ... */ }

and with TryOnce, it would be

var loadStateOnce sync.TryOnce[*Thing]

func State() (*Thing, error) {
	return loadStateOnce.Do(loadState)
}

func loadState() (*Thing, error) { /* ... */ }

Neither has a real advantage in terms of conciseness. Adjusting either of these to get rid of the once-ness would be trivial.

One thing I notice about OnceFunc is that it might be tempting to save a few lines and write:

var State = sync.OnceFunc(loadState)

func loadState() (*Thing, error) { /* ... */ }

I think this would be a mistake, though. So both in your original example and here, OnceFunc seems to promote the use of function values in places where we would idiomatically use methods or normal functions in Go. OnceFunc seems like a function that would be very much at home in the functional languages I use, but not Go. (For instance it is available as memoize on a 0-ary function in Clojure.)

@adg
Copy link
Contributor Author

adg commented Oct 10, 2022

Actually the case in which I use it is globals; I chose the struct form as it's as slightly more complex context.

This is how you would use OnceFunc for your state example:

var state = sync.OnceFunc(func() (*Thing, error) {
        /* ... */
}

This IMO is way more concise than any of the alternatives.

(Sorry sent this before I had finished writing it.)

I think that function variables are fine in the context of private globals. If you wanted to export it and protect it from mutation outside the package, you could write:

func State() (*Thing, error) { return loadState() }

var loadState = sync.OnceFunc(func() (*Thing, error) {
        /* ... */
})

OnceFunc seems to promote the use of function values in places where we would idiomatically use methods or normal functions in Go.

I think this kind of argument is not vey helpful in this context. We have new possibilities with generics; we should argue on the pros/cons, not by the established practices that pre-date the tools we have available today.

@cespare
Copy link
Contributor

cespare commented Oct 10, 2022

I think this kind of argument is not vey helpful in this context. We have new possibilities with generics; we should argue on the pros/cons, not by the established practices that pre-date the tools we have available today.

Ah -- I'd been taking it as self-evident that promoting function variables over functions should be avoided. My mistake.

I think that replacing functions with variables is a poor practice on the merits:

  • Having multiple ways to do things is a burden on code writers and code readers. (As an occasional JavaScript writer I never know whether I should be declaring my functions using function or var.)
  • The fact that a var is mutable means the reader of the code doesn't know whether the value has changed without locating all references to the var.
  • The fact that a var is mutable opens the possibility of a data race if it is modified elsewhere.
  • Using vars instead of functions further encourages mutating the var for testing purposes, an ill-advised practice that interferes with test parallelization and makes debugging harder.
  • Invoking a function value through a var is slower than calling a function.
  • The stack trace you get when calling a function through a var is less helpful (it doesn't include the var location).

Therefore, I think that APIs should not encourage using vars where functions or methods would have traditionally been used, and thus I think that a struct type is a better API here than a higher-order function.

@adg
Copy link
Contributor Author

adg commented Oct 10, 2022

I'd been taking it as self-evident that promoting function variables over functions should be avoided.

I think this is true, for most uses of global function variables, and most certainly exported ones. I'd go further to suggest that global variables as a whole are best avoided where possible.

But I don't think function variables should be avoided as a whole. For instance, look at this pitfall you describe:

Using vars instead of functions further encourages mutating the var for testing purposes, an ill-advised practice that interferes with test parallelization and makes debugging harder.

One good solution to this is to actually use a function variable, rather than mutating global state. For example, if you want to test some code that works with time, passing it a mock now func() time.Time (either as a function argument or by setting a struct field) allows you to control precisely what that function does in your tests.

So from here I'm assuming that function variables do have some value, and should be used where appropriate.

In the context of this proposal, where OnceFunc might be used to set a global function variable it would be replacing the use of three global variables. From your example:

var state struct {
	once sync.Once
	val *Thing
	err error
}

I think, on the whole, if you're writing programs with globals like this then you're already vulnerable to most of the pitfalls you describe. If anything, OnceFunc makes it harder to make a mess of it. (This is why I chose to use the Server struct example, btw.)

Other concerns you raised:

Invoking a function value through a var is slower than calling a function.

That may be true in isolation but we'd need to benchmark the different approaches described here to make any efficiency arguments.

The stack trace you get when calling a function through a var is less helpful (it doesn't include the var location).

These stack traces seem equally helpful to me: before and after

@rsc
Copy link
Contributor

rsc commented Oct 12, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Oct 12, 2022
@rsc
Copy link
Contributor

rsc commented Oct 20, 2022

This is clearly a very common operation that we should make easier. The only discussion seems to be whether to include the error in the function signature. So maybe there should be two forms: one with the error and one without. Generalizing "with error" to two values may make sense. But if so, what are the names? OnceVal / OnceError and Once1 / Once2 both seem a bit strange. Maybe we should find a name that's not "Once"?

sync.Lazy[T], sync.Lazy2[T] - lazy is maybe overused, or maybe it should have the function ahead of time
sync.Memo[T], sync.Memo2[T] - memoizing is usually parameterized, and this isn't
sync.Cache[T], sync.Cache2[T] - but caches can be cleared, and this can't

So lots of ideas, none of them great.

@icholy
Copy link

icholy commented Oct 20, 2022

What about a single type with 2 methods:

type Memo[T any] struct {
	val  T
	err  error
	once sync.Once
}

func (m *Memo[T]) Do(f func() T) T {
	m.once.Do(func() {
		m.val = f()
	})
	return m.val
}

func (m *Memo[T]) DoErr(f func() (T, error)) (T, error) {
	m.once.Do(func() {
		m.val, m.err = f()
	})
	return m.val, m.err
}

@DeedleFake
Copy link

This reminds me of #37739, but implemented with generics instead of as a language change and tailored specifically for use with concurrency. Maybe it makes sense to split the concurrency support out? In other words, have a lazy value interface somewhere non-concurrency specific and a sync.Lazy struct that wraps it to make it thread-safe?

package something

type Lazy[T any] interface {
  Eval() T
}
package sync

type Lazy[T any] struct {
  lazy something.Lazy[T]
}

@rsc
Copy link
Contributor

rsc commented Oct 26, 2022

To try to move things forward, what do people think of

type OnceValue[T any] struct { ... }
func (*OnceValue[T]) Do(func() T) T 

type OnceValueErr[T any] struct { ... }
func (*OnceValueErr[T]) Do(func() (T, error)) (T, error)

?

It seems like the name should begin with Once so that people find it when they are looking for sync.Once.

@icholy
Copy link

icholy commented Oct 26, 2022

I think that two types is overkill. People can easily ignore the error return if they don't need it.

edit: I like the two type approach the best.

@earthboundkid
Copy link
Contributor

I prefer icholy's suggestion of one type with two methods to two types.

@zephyrtronium
Copy link
Contributor

zephyrtronium commented Oct 26, 2022

Having one type with two methods leaves open the possibility to use both with one OnceValue. That seems like a source of bugs. I can imagine legitimate ways to use both methods together, but the (T, error) form subsumes them.

@icholy
Copy link

icholy commented Oct 26, 2022

Just to sum it up, here are the variations:

One type, one method:

type OnceValue[T any] struct { ... }
func (*OnceValue[T]) Do(func() (T, error)) (T , error)

Two types, one method:

type OnceValue[T any] struct { ... }
func (*OnceValue[T]) Do(func() T) T 

type OnceValueErr[T any] struct { ... }
func (*OnceValueErr[T]) Do(func() (T, error)) (T, error)

One type, two methods:

type OnceValue[T any] struct { ... }
func (*OnceValue[T]) Do(func() T) T 
func (*OnceValue[T]) DoErr(func() (T, error)) (T, error)

@cespare
Copy link
Contributor

cespare commented Oct 26, 2022

The one type, two methods version seems fine to me. To prevent misuse Do could panic if DoErr was previously called.

@rsc
Copy link
Contributor

rsc commented Oct 26, 2022

One type, two methods seems out-of-place, because you have to call one of the methods consistently to use it correctly.
If there are goroutines racing (this is package sync) and one calls Do and the other calls DoErr, then in at least one case we have a problem: when DoErr wins, caches an error, and then Do runs and can't return the error. That suggests that any valid use should either always call Do or always call DoErr. To avoid latent bugs that only show up in production, the implementation should probably panic any time it observes both methods being used.

So really there are two kinds of OnceValue: the kind that can only use Do, and the kind that can only use DoErr. Giving them the same Go type means the compiler can't help you make sure you are using the type correctly. In contrast, what we usually do in Go is use different types for different kinds of values, and then the type system and the compiler do help you, and this possible runtime panic is eliminated at compile time.

I think that excludes "one type, two methods".

"One type, one method" seems not quite right, because sometimes we will be caching things that can't possibly fail, and it's annoying to have to discard the error that can't happen anyway. Yes, code that can fail is common, but so is code that can't fail.

That leaves "two types, one method", which is why I suggested OnceValue and OnceValueErr.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/479095 mentions this issue: cmd/compile: allow more inlining of functions that construct closures

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/481062 mentions this issue: sync: add examples for OnceValue and OnceValues

gopherbot pushed a commit that referenced this issue Mar 31, 2023
Currently, when the inliner is determining if a function is
inlineable, it descends into the bodies of closures constructed by
that function. This has several unfortunate consequences:

- If the closure contains a disallowed operation (e.g., a defer), then
  the outer function can't be inlined. It makes sense that the
  *closure* can't be inlined in this case, but it doesn't make sense
  to punish the function that constructs the closure.

- The hairiness of the closure counts against the inlining budget of
  the outer function. Since we currently copy the closure body when
  inlining the outer function, this makes sense from the perspective
  of export data size and binary size, but ultimately doesn't make
  much sense from the perspective of what should be inlineable.

- Since the inliner walks into every closure created by an outer
  function in addition to starting a walk at every closure, this adds
  an n^2 factor to inlinability analysis.

This CL simply drops this behavior.

In std, this makes 57 more functions inlinable, and disallows inlining
for 10 (due to the basic instability of our bottom-up inlining
approach), for an net increase of 47 inlinable functions (+0.6%).

This will help significantly with the performance of the functions to
be added for #56102, which have a somewhat complicated nesting of
closures with a performance-critical fast path.

The downside of this seems to be a potential increase in export data
and text size, but the practical impact of this seems to be
negligible:

               │    before    │           after            │
               │    bytes     │    bytes      vs base      │
Go/binary        15.12Mi ± 0%   15.14Mi ± 0%  +0.16% (n=1)
Go/text          5.220Mi ± 0%   5.237Mi ± 0%  +0.32% (n=1)
Compile/binary   22.92Mi ± 0%   22.94Mi ± 0%  +0.07% (n=1)
Compile/text     8.428Mi ± 0%   8.435Mi ± 0%  +0.08% (n=1)

Change-Id: Ie9e38104fed5689a94c368288653fd7cb4b7a35e
Reviewed-on: https://go-review.googlesource.com/c/go/+/479095
Reviewed-by: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
@github-project-automation github-project-automation bot moved this from Todo to Done in Go Compiler / Runtime Mar 31, 2023
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Mar 31, 2023
@thanm
Copy link
Contributor

thanm commented Apr 3, 2023

FYI: https://go.dev/cl/479095 had to be rolled back, due to problems encountered during google-internal testing (see #59404). Working now on resolving that.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/482356 mentions this issue: cmd/compile: allow more inlining of functions that construct closures

gopherbot pushed a commit that referenced this issue Apr 7, 2023
[This is a roll-forward of CL 479095, which was reverted due to a bad
interaction between inlining and escape analysis since fixed in CL 482355.]

Currently, when the inliner is determining if a function is
inlineable, it descends into the bodies of closures constructed by
that function. This has several unfortunate consequences:

- If the closure contains a disallowed operation (e.g., a defer), then
  the outer function can't be inlined. It makes sense that the
  *closure* can't be inlined in this case, but it doesn't make sense
  to punish the function that constructs the closure.

- The hairiness of the closure counts against the inlining budget of
  the outer function. Since we currently copy the closure body when
  inlining the outer function, this makes sense from the perspective
  of export data size and binary size, but ultimately doesn't make
  much sense from the perspective of what should be inlineable.

- Since the inliner walks into every closure created by an outer
  function in addition to starting a walk at every closure, this adds
  an n^2 factor to inlinability analysis.

This CL simply drops this behavior.

In std, this makes 57 more functions inlinable, and disallows inlining
for 10 (due to the basic instability of our bottom-up inlining
approach), for an net increase of 47 inlinable functions (+0.6%).

This will help significantly with the performance of the functions to
be added for #56102, which have a somewhat complicated nesting of
closures with a performance-critical fast path.

The downside of this seems to be a potential increase in export data
and text size, but the practical impact of this seems to be
negligible:

	       │    before    │           after            │
	       │    bytes     │    bytes      vs base      │
Go/binary        15.12Mi ± 0%   15.14Mi ± 0%  +0.16% (n=1)
Go/text          5.220Mi ± 0%   5.237Mi ± 0%  +0.32% (n=1)
Compile/binary   22.92Mi ± 0%   22.94Mi ± 0%  +0.07% (n=1)
Compile/text     8.428Mi ± 0%   8.435Mi ± 0%  +0.08% (n=1)

Updates #56102.

Change-Id: I1f4fc96c71609c8feb59fecdb92b69ba7e3b5b41
Reviewed-on: https://go-review.googlesource.com/c/go/+/482356
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/484860 mentions this issue: cmd/compile: allow more inlining of functions that construct closures

gopherbot pushed a commit that referenced this issue Apr 17, 2023
[This is a roll-forward of CL 479095, which was reverted due to a bad
interaction between inlining and escape analysis, then later fixed
fist with an attempt in CL 482355, then again in 484859 .]

Currently, when the inliner is determining if a function is
inlineable, it descends into the bodies of closures constructed by
that function. This has several unfortunate consequences:

- If the closure contains a disallowed operation (e.g., a defer), then
  the outer function can't be inlined. It makes sense that the
  *closure* can't be inlined in this case, but it doesn't make sense
  to punish the function that constructs the closure.

- The hairiness of the closure counts against the inlining budget of
  the outer function. Since we currently copy the closure body when
  inlining the outer function, this makes sense from the perspective
  of export data size and binary size, but ultimately doesn't make
  much sense from the perspective of what should be inlineable.

- Since the inliner walks into every closure created by an outer
  function in addition to starting a walk at every closure, this adds
  an n^2 factor to inlinability analysis.

This CL simply drops this behavior.

In std, this makes 57 more functions inlinable, and disallows inlining
for 10 (due to the basic instability of our bottom-up inlining
approach), for an net increase of 47 inlinable functions (+0.6%).

This will help significantly with the performance of the functions to
be added for #56102, which have a somewhat complicated nesting of
closures with a performance-critical fast path.

The downside of this seems to be a potential increase in export data
and text size, but the practical impact of this seems to be
negligible:

	       │    before    │           after            │
	       │    bytes     │    bytes      vs base      │
Go/binary        15.12Mi ± 0%   15.14Mi ± 0%  +0.16% (n=1)
Go/text          5.220Mi ± 0%   5.237Mi ± 0%  +0.32% (n=1)
Compile/binary   22.92Mi ± 0%   22.94Mi ± 0%  +0.07% (n=1)
Compile/text     8.428Mi ± 0%   8.435Mi ± 0%  +0.08% (n=1)

Updates #56102.

Change-Id: I6e938d596992ffb473cf51e7e598f372ce08deb0
Reviewed-on: https://go-review.googlesource.com/c/go/+/484860
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@mknyszek
Copy link
Contributor

CL 484860 broke the x/benchmarks builder: https://build.golang.org/?repo=golang.org%2fx%2fbenchmarks

Failure: https://build.golang.org/log/11803116ada45552b52ed62dd86f4255cdba5d87

What's happening is this call produces a closure that appears to be inlined into a compiler-generated init function. The closure it produces later gets installed here.

The end result is this function ends up dereferencing a nil pointer.

@mknyszek mknyszek reopened this Apr 17, 2023
@github-project-automation github-project-automation bot moved this from Done to In Progress in Go Compiler / Runtime Apr 17, 2023
@mknyszek
Copy link
Contributor

I reopened the issue in case we want to roll back this CL, but feel free to close and I can file a new issue instead.

@mknyszek
Copy link
Contributor

Nevermind, filed #59680. Closing this issue.

@github-project-automation github-project-automation bot moved this from In Progress to Done in Go Compiler / Runtime Apr 17, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/492017 mentions this issue: cmd/compile: allow more inlining of functions that construct closures

gopherbot pushed a commit that referenced this issue May 5, 2023
[This is a roll-forward of CL 479095, which was reverted due to a bad
interaction between inlining and escape analysis, then later fixed
first with an attempt in CL 482355, then again in CL 484859, and then
one more time with CL 492135.]

Currently, when the inliner is determining if a function is
inlineable, it descends into the bodies of closures constructed by
that function. This has several unfortunate consequences:

- If the closure contains a disallowed operation (e.g., a defer), then
  the outer function can't be inlined. It makes sense that the
  *closure* can't be inlined in this case, but it doesn't make sense
  to punish the function that constructs the closure.

- The hairiness of the closure counts against the inlining budget of
  the outer function. Since we currently copy the closure body when
  inlining the outer function, this makes sense from the perspective
  of export data size and binary size, but ultimately doesn't make
  much sense from the perspective of what should be inlineable.

- Since the inliner walks into every closure created by an outer
  function in addition to starting a walk at every closure, this adds
  an n^2 factor to inlinability analysis.

This CL simply drops this behavior.

In std, this makes 57 more functions inlinable, and disallows inlining
for 10 (due to the basic instability of our bottom-up inlining
approach), for an net increase of 47 inlinable functions (+0.6%).

This will help significantly with the performance of the functions to
be added for #56102, which have a somewhat complicated nesting of
closures with a performance-critical fast path.

The downside of this seems to be a potential increase in export data
and text size, but the practical impact of this seems to be
negligible:

	       │    before    │           after            │
	       │    bytes     │    bytes      vs base      │
Go/binary        15.12Mi ± 0%   15.14Mi ± 0%  +0.16% (n=1)
Go/text          5.220Mi ± 0%   5.237Mi ± 0%  +0.32% (n=1)
Compile/binary   22.92Mi ± 0%   22.94Mi ± 0%  +0.07% (n=1)
Compile/text     8.428Mi ± 0%   8.435Mi ± 0%  +0.08% (n=1)

Change-Id: I5f75fcceb177f05853996b75184a486528eafe96
Reviewed-on: https://go-review.googlesource.com/c/go/+/492017
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
eric pushed a commit to fancybits/go that referenced this issue Sep 2, 2023
This adds the three functions from golang#56102 to the sync package. These
provide a convenient API for the most common uses of sync.Once.

The performance of these is comparable to direct use of sync.Once:

$ go test -run ^$ -bench OnceFunc\|OnceVal -count 20 | benchstat -row .name -col /v
goos: linux
goarch: amd64
pkg: sync
cpu: 11th Gen Intel(R) Core(TM) i7-1185G7 @ 3.00GHz
          │     Once     │                Global                 │                Local                 │
          │    sec/op    │    sec/op     vs base                 │    sec/op     vs base                │
OnceFunc    1.3500n ± 6%   2.7030n ± 1%  +100.22% (p=0.000 n=20)   0.3935n ± 0%  -70.86% (p=0.000 n=20)
OnceValue   1.3155n ± 0%   2.7460n ± 1%  +108.74% (p=0.000 n=20)   0.5478n ± 1%  -58.35% (p=0.000 n=20)

The "Once" column represents the baseline of how code would typically
express these patterns using sync.Once. "Global" binds the closure
returned by OnceFunc/OnceValue to global, which is how I expect these
to be used most of the time. Currently, this defeats some inlining
opportunities, which roughly doubles the cost over sync.Once; however,
it's still *extremely* fast. Finally, "Local" binds the returned
closure to a local variable. This unlocks several levels of inlining
and represents pretty much the best possible case for these APIs, but
is also unlikely to happen in practice. In principle the compiler
could recognize that the global in the "Global" case is initialized in
place and never mutated and do the same optimizations it does in the
"Local" case, but it currently does not.

Fixes golang#56102

Change-Id: If7355eccd7c8de7288d89a4282ff15ab1469e420
Reviewed-on: https://go-review.googlesource.com/c/go/+/451356
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Andrew Gerrand <adg@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Caleb Spare <cespare@gmail.com>
Auto-Submit: Austin Clements <austin@google.com>
@rsc rsc removed this from Proposals Apr 18, 2024
gopherbot pushed a commit that referenced this issue Apr 23, 2024
Updates #56102.

Change-Id: I2ee2dbc43b4333511d9d23752fdc574dfbf5f5bd
Reviewed-on: https://go-review.googlesource.com/c/go/+/481062
Reviewed-by: Andrew Gerrand <adg@golang.org>
Auto-Submit: Austin Clements <austin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Joedian Reid <joedian@google.com>
@golang golang locked and limited conversation to collaborators May 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests