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: optimized range memclr doesn't work with pointers to arrays #52635

Closed
josharian opened this issue Apr 30, 2022 · 7 comments
Closed

Comments

@josharian
Copy link
Contributor

package p

type T struct {
	a *[10]int
	b [10]int
}

func (t *T) resetA() {
	for i := range t.a {
		t.a[i] = 0
	}
}

func (t *T) resetB() {
	for i := range t.b {
		t.b[i] = 0
	}
}

resetA compiles to a loop. resetB compiles to a call to runtime.memclrNoHeapPointers. I believe that resetA should also call memclrNoHeapPointers.

This showed up as a hot spot in my code today (with much larger arrays).

cc @randall77 @cuonglm @mdempsky

@josharian
Copy link
Contributor Author

As evidence that this is just a missed optimization, here is equivalent code using a new intermediate type:

type T struct {
	c *A
}

type A struct {
	x [10]int
}

func (t *T) resetC() {
	for i := range t.c.x {
		t.c.x[i] = 0
	}
}

This generates runtime.memclrNoHeapPointers. (And also provides a workaround.)

@renthraysk
Copy link

There is a simpler workaround for i := range *t.a {.

@josharian
Copy link
Contributor Author

josharian commented May 1, 2022

@renthraysk I am astonished that that works, and I wrote the original optimization. Now that you say it, I can imagine why, although it has to do with weird quirks in the guts of the compiler. Anyway, I still think this is worth fixing. :P

@renthraysk
Copy link

Yea, agreed. Actually thought I filed an issue for this, but seems just tweeted about it back in 2017, so my bad.

Subtle diff in
func f1(in *[96]byte) { for i := range in { in[i] = 0 } }
func f2(in *[96]byte) { for i := range *in { in[i] = 0 } }

@gopherbot
Copy link

Change https://go.dev/cl/403337 mentions this issue: cmd/compile: support pointers to arrays in arrayClear

@rip-create-your-account
Copy link

The following generates slightly more efficient code for my computer.

func (t *T) resetC() {
	*t.a = [10]int{}
}

There's no runtime.memclrNoHeapPointers call at all. Instead the compiler seems to choose a suitable algorithm based on the size of the array. To me it seems like all of the loops above should compile to this.

@go101
Copy link

go101 commented May 2, 2022

It looks the memclr way is more efficient than the assignment way for small arrays.
Should the runtime internal unify the implementation for the two ways?

@golang golang locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants