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

cmd/compile: optimized range memclr doesn't work with pointers to arrays #52635

josharian opened this issue Apr 30, 2022 · 7 comments

Comments

@josharian
Copy link
Contributor

@josharian josharian commented Apr 30, 2022

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

@josharian josharian commented Apr 30, 2022

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

@renthraysk renthraysk commented May 1, 2022

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

@josharian
Copy link
Contributor Author

@josharian 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

@renthraysk renthraysk commented May 1, 2022

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

@gopherbot gopherbot commented May 1, 2022

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

@rip-create-your-account
Copy link

@rip-create-your-account rip-create-your-account commented May 1, 2022

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 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants