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: checking pointers for nilness inside a loop #41666

Open
seebs opened this issue Sep 28, 2020 · 4 comments
Open

cmd/compile: checking pointers for nilness inside a loop #41666

seebs opened this issue Sep 28, 2020 · 4 comments

Comments

@seebs
Copy link
Contributor

@seebs seebs commented Sep 28, 2020

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

$ go version
1.15

Does this issue reproduce with the latest release?

y

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

amd64

What did you do?

Tried to optimize a function by replacing a slice parameter with known size with a *[N]T parameter, in principle reducing the number of things passed to the function.

https://godbolt.org/z/hz68Mn

What did you expect to see?

I'm honestly not sure, the whole exercise was perhaps doomed from the start.

What did you see instead?

There's extra tests. Well, not quite extra. But there's a test al, cx (spelled slightly differently in pprof output) before the first access to a given array in the function, which happens to be inside a loop, meaning that the test gets hit hundreds of times. Of course, it can't just be hoisted out of the loop in full generality -- if the loop only sometimes got hit, hitting the test before the loop would be bad, and if there are side-effects or something inside the loop, you don't want to panic before them. The corresponding slice case doesn't need this, because a slice with a nil pointer also has a 0 len and thus the loop doesn't get entered.

Adding _ = l1[0] (for each value) to the top of the function does eliminate these.

I'm not sure whether this could be trivially fixed but it feels like a lot of tests to keep testing for nilness for a pointer every time you use it...

@mvdan mvdan added the Performance label Sep 28, 2020
@andybons andybons added this to the Unplanned milestone Sep 29, 2020
@randall77
Copy link
Contributor

@randall77 randall77 commented Sep 29, 2020

You're right, we can't move the nil check because we don't know how many side effects happen before we check it. Keeping track of whether we did the nil check is probably going to be more expensive than just doing the check again.

There are probably many situations where the nilness is checked unconditionally before any side effects. We could lift the nil check out of the loop for those cases. If the nilness is checked unconditionally, but after some side effects, we could peel the first iteration of the loop. Or we could have two loops, one with nil checks and one without, and jump from the former to the latter once the nilness is checked. There are definitely diminishing returns here, though.

Your technique of adding a _ = x[0] is fine. We do a similar thing for bounds checks, _ = s[7] to ensure there are at least 8 elements in a slice.

@seebs
Copy link
Contributor Author

@seebs seebs commented Sep 29, 2020

Yeah, I thought briefly about some kind of fancy duff's-device-like thing where there's a duplicate loop body in front of the actual loop, and the first instance has the nil check, but that's clearly awful.

@dr2chase
Copy link
Contributor

@dr2chase dr2chase commented Sep 29, 2020

That's not at all "clearly awful", I've worked on compilers that did this. The question is, "which loops"? You need to ensure that the always-taken portion of the loop is most of the loop, and the fraction of loop-invariant true nil checks in that always-taken portion of the loop is large enough. Also applies to array bounds, etc. It may not align with the default policies for adding optimization to the Go compiler, that is true.

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

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.