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: range loops on slices of large types possibly surprisingly expensive #31588

Closed
seebs opened this issue Apr 20, 2019 · 6 comments

Comments

Projects
None yet
4 participants
@seebs
Copy link
Contributor

commented Apr 20, 2019

What version of Go are you using (go version)?

1.12

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

amd64

What did you do?

for _, v := range sliceOfStructs {
// access a single member of v
}

contrast

for i := range sliceOfStructs {
// access a single member of sliceOfStructs[i]
}

What did you expect to see?

Less copying. I don't know why I expected that.

What did you see instead?

The slice range loop is by-value; it copies each item in a slice. It copies the entire item, even if only one member of it is actually accessed.

Idiomatically, it's cleaner to express this as "for each value in the list", but the performance penalty can be large. (The example I saw given was a bit over a factor of 2 total performance cost for the loop, since the code inside the loop was trivial.)

I'm not sure whether this is fixable. If we wanted to reinvent C++, there could just be a syntax for "range loop, but you actually operate on the members of the slice". (Or map? Probably not, since they're not addressable.) But I could imagine the compiler looking at the contents, establishing that only one item is being accessed, and just copying that one item.

@go101

This comment has been minimized.

Copy link

commented Apr 21, 2019

Yes, it is expensive, and it is a fact, not possibly surprisingly.

@seebs

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

to clarify, my intent was to express that the amount of expense might be surprising, not that it might be (or might not be) expensive.

@dsnet dsnet added the Performance label Apr 21, 2019

@go101

This comment has been minimized.

Copy link

commented Apr 21, 2019

It is not surprising. Would you want compilers to optimize the code?

@josharian

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2019

@go101 please keep your comments constructive.

@seebs under the hood, the compiler makes a copy of the value so that subsequent code can safely modify that value without side-effects. I would expect that the extra copy would be optimized away in most cases. The exception would be cases in which we treat the value as opaque because it is large (struct with > 4 fields, array with > 1 field). If that is accurate, then this is probably effectively a duplicate of #24416.

@seebs

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

Huh, I didn't even think to check it with small structs. So looking around, the original case involved a slice member, and experimenting on godbolt, it seems to me that a slice counts as multiple members for purposes of that optimization. A struct with four int64 members, only one of which is accessed, doesn't show the whole copy, but five do. But if I have a []byte and two int64s, it copies. which I guess makes sense; a slice is three things.

And yes, i would want compilers to optimize the code, where possible. I value performance, and I value clarity, and I do not like having to choose between them.

I keep thinking about this and inventing range-by-reference, and thinking "gosh, i wish i had that". and then thinking "gosh, i sure hope no one else whose code i have to maintain ever gets that." And realistically, the worst other programmer whose code i've had to deal with has usually been "me a few months ago" so probably I don't actually want it.

To clarify: Yes, I think this is a duplicate of #24416. Thanks for spotting that.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2019

Cool. I'm going to go ahead and close this as a dup of #24416. Thanks for reporting it.

@josharian josharian closed this Apr 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.