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

cmd/compile: storing zero value into interface should not allocate #71323

Open
dsnet opened this issue Jan 19, 2025 · 8 comments
Open

cmd/compile: storing zero value into interface should not allocate #71323

dsnet opened this issue Jan 19, 2025 · 8 comments
Assignees
Labels
BugReport Issues describing a possible bug in the Go implementation. compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@dsnet
Copy link
Member

dsnet commented Jan 19, 2025

Go version

go1.23.5

Output of go env in your module/workspace:

GOARCH=amd64
GOOS=linux

What did you do?

I ran this benchmark:

var sinkAny any

func BenchmarkInterfaceAny(b *testing.B) {
	b.ReportAllocs()
	for range b.N {
		sinkAny = tar.Header{}
	}
}

var sinkValue reflect.Value

func BenchmarkReflectValue(b *testing.B) {
	b.ReportAllocs()
	for range b.N {
		sinkValue = reflect.Zero(reflect.TypeFor[tar.Header]())
	}
}

What did you see happen?

I saw these results:

BenchmarkInterfaceAny    	31895619	        43.36 ns/op	     224 B/op	       1 allocs/op
BenchmarkReflectValue    	401489182	         3.021 ns/op	       0 B/op	       0 allocs/op

What did you expect to see?

I expected to see zero allocations in both cases.

In #33136, we optimized reflect.Zero to use a zero buffer of sufficient size to back the zero value of most types.

It seems to me that we could do the same trick for the non-reflect equivalent of storing a zero value into an interface value, where the data pointer in the interface value points to some shared zero buffer. This can be done in two ways:

  1. Dynamically check whether the value being stored into an interface is the zero value
  2. Statically detect when the value being stored into the interface value is identically equivalent to the zero value

Of course in both cases, you'll need to be careful since we're talking about a stricter definition of "zero" where the memory is equivalent to be all zeros.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jan 19, 2025
@dsnet dsnet added Performance compiler/runtime Issues related to the Go compiler and/or runtime. and removed compiler/runtime Issues related to the Go compiler and/or runtime. labels Jan 19, 2025
@gabyhelp gabyhelp added the BugReport Issues describing a possible bug in the Go implementation. label Jan 19, 2025
@randall77
Copy link
Contributor

Dynamically check whether the value being stored into an interface is the zero value

This would be in runtime/iface.go, convT and convTnoptr. I'm not sure I want to check for zeroness on every allocation? It seems a low probability thing in most cases. Then again, the cost of mallocgc+typedmemmove is pretty large.

Statically detect when the value being stored into the interface value is identically equivalent to the zero value

That would be efficient at runtime.

How often does this come up? It seems like a pretty rare thing.

@thepudds
Copy link
Contributor

Hi @dsnet, as you're probably aware, for basic types like strings/ints/floats, the runtime already does avoid allocations for interface conversions by dynamically detecting the zero value.

I think the runtime does not do that for structs, though. One thing is that re-using the runtime's zeroVal memory for reflect.Zero I think might be a pure win, whereas checking a struct at runtime for an interface conversion would not be a pure win because most often it would not find the zero value but would incur some performance penalty to do the check dynamically. It might also need to be careful about what named types can do, though maybe that falls out naturally.

Separately, I think your example might be addressed by #62653, which is a more general solution that does not rely on whether or not the struct value is the zero value. (See intro comment in #62653, or #8618 (comment) for some related context).

One question for you: as you were thinking about writing up this issue, are most of your real-world use cases for something like this issue really looking to store something in a global interface value (as shown in your simple benchmark), vs. is it more often trying to avoid an escape to the heap for something that could be otherwise stack allocated?

In any event, it happens that I was separately looking into a more narrow case of statically detecting struct literals being used in interface conversions, which I think might address your specific example (more narrowly than #62653 would). I'll take a look at your example in that context.

@zigo101
Copy link

zigo101 commented Jan 20, 2025

Maybe, it is always not a good idea to store big structs in interfaces.
Doing this will always store an implicit pointer in interfaces.
We can store an explicit pointer instead.

var sinkAny any = &tar.Header{}

{
    *sinkAny.(*tar.Header) = tar.Header{}
}

@adonovan
Copy link
Member

As a quick experiment I ran this snippet on each go/ssa basic block b over non-test code in std and cmd/...:

		var lastPos token.Pos // approximate position
		for _, instr := range b.Instrs {
			if pos := instr.Pos(); pos != 0 {
				lastPos = pos
			}
			switch instr := instr.(type) {
			case *ssa.MakeInterface:
				if k, ok := instr.X.(*ssa.Const); ok && k.Value == nil {
					log.Printf("%s: (%s)(zero[%s]()) -- %s",
						b.Parent().Prog.Fset.Position(lastPos),
						instr.Type(),
						instr.X.Type(),
						strings.TrimPrefix(reflect.TypeOf(instr.X.Type().Underlying()).String(), "*types."),
					)
				}
			}
		}

and it turned up this histogram of operand types for a MakeInterface operation with a "constant" zero operand:

  561 Struct
  57 Slice
  34 Pointer
  13 Map
  12 Signature
   8 Array
   4 Chan

So Struct is actually surprisingly common (though I suspect many are zero-sized structs).

The file is attached, with approximate file/line/column info for some instances.

log.txt

@zigo101
Copy link

zigo101 commented Jan 21, 2025

Slices and most arrays are also non-small-size types.

Just realized that OP's suggestion is dangerous? It is possible that sinkAny is still referenced elsewhere, changing its value will break the expectations elsewhere.

I'm wondering that whether or not the case is similar to reflect.Value values.

@zigo101
Copy link

zigo101 commented Jan 21, 2025

Er, my concern posted in the last comment is not valid.

@thepudds
Copy link
Contributor

I have a first cut of statically detecting a composite value can be represented by all zeros, and if so, use a pointer to zeroVal at compile time instead of a heap allocation. It at least passes some very basic tests, including it now reports zero allocs for @dsnet's benchmark from above:

$ go test -bench=.

pkg: cmd/compile/internal/escape/_test/dsnet-benchmark
BenchmarkInterfaceAny-4         1000000000               0.8450 ns/op          0 B/op          0 allocs/op
BenchmarkReflectValue-4         96306639                12.46 ns/op            0 B/op          0 allocs/op
PASS

I wouldn't be surprised if I'm currently doing something in the wrong spot or the wrong way, but I'll try to clean it up and send a CL for consideration in the not too distant future.

The change uses ir.IsZero and is mostly in walk. It builds on some things I had been experimenting with a few months ago around improving how escape analysis treats local variables that flow from literals, especially when they are used as length/capacity arguments to make or in interface conversions (#71359).

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 21, 2025
@thepudds thepudds self-assigned this Jan 21, 2025
@mknyszek mknyszek added this to the Unplanned milestone Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BugReport Issues describing a possible bug in the Go implementation. compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
Development

No branches or pull requests

9 participants