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: unexpected prove/BCE regressions when building encoding/json #31275
Comments
Are you able to run a bisect? |
Bisected: 83a33d3 is the first bad commit |
/cc @randall77 as FYI |
It looks like 83a33d3 changed the sequence of checks in sliceBoundsCheck from:
to
I've debugged the case above from encode.go:728, and prove needs to learn The bisected CL both removed and reordered the checks, and both changes would need to be reversed in order to make prove, as it is, able to remove the OpIsSliceInBounds. I need to look more at what prove needs to do to remove these going forward. |
I fail to understand how the current code manages to check that j or k are non negative. @randall77? |
For comparison, it seems like the jump from Go 1.11.5 to 1.12.1 removed three bounds checks, and added one in autogenerated code. Also, for what it's worth, I'm not implying that this causes some benchmarks to get slower; I was just surprised by some bounds checks I could have sworn weren't there last cycle. Assuming it was an unintended regression, we probably want to fix it and add some tests. |
The current checks are more correctly described as:
The reason why we do these in reverse order is that the opcode we use to check |
@mvdan can you post assembly code for one of those occurrences before and after?
go/src/encoding/json/encode.go Lines 724 to 731 in 06cff11
Since |
The same applies to go/src/encoding/json/scanner.go Lines 147 to 152 in cb66462
We can't remove the bound check here. It sounds weird that we're doing it in Go 1.12.1, though. I do get a out-of-bound error if I try to reproduce it: |
@randall77, apologies, I should have been more precise. I didn't mean that to sound as broad as it did. More precisely: The explicit
@rasky, that was my first thought when I checked the examples. That's why I dug deeper, because it didn't seem like prove should have been removing those bounds checks. The inserted checks were ensuring those were non-negative. See SSA snippets below. 3cf89e5... the commit before the change.
83a33d3... the commit
Full ssa dumps Debug output from each: |
Perhaps you're right that the old compiler was buggy in these cases; I didn't investigate the details at all. If that's the case, and we fixed them without even noticing, we probably want to add a regression test. |
Right. It's partly just a matter of accounting: we're removing a |
I See. What I'm understanding then: OpIsSliceInBounds will ultimately be three instructions, each of which will check both the upper and lower bounds of one of In the previous implementation, in addition to actually doing the bounds check, we needed to first check whether each upper bound value was non-negative. The non-negative check(s) was an additional OpGeq64 in the SSA, just before the OpSliceIsInBounds proper. Prove was learning from that additional op (which, like you said, is actually half of the bounds check) before arriving at the OpIsSliceInBounds, at which point it was legitimately able to remove the bounds check as a whole in the examples at hand. In the current implementation, because we know that the cap(s) upper bound is guaranteed non-negative, we can bootstrap that guarantee down the chain of checks. That means the non-negative check is no longer necessary, and has therefore been removed. The slice bounds check, in it's entirety, is now represented by just the OpIsSliceInBounds in the SSA. @randall77 As an aside because I'd like to learn more about it: which instruction do we use to check both the lower and upper bounds? Is it just done as an unsigned comparison? |
IsSliceInBounds is a single instruction. For
Exactly.
Yes. |
I think we can close this issue at this point - I spotted a potential prove regression, but after some discussion that doesn't seem to be the case. And this issue is pretty old now, so I don't think it's useful anymore. Thanks all for the digging :) |
So it seems like BCE is actually getting worse in 1.13 for this particular package. It has over a hundred bounds checks, a dozen of which are in hot decode functions, so I'd like for the number to go down, not up :)
I used
encoding/json
from the master version on both cases, to make the comparison fair and simple./cc @zdjones @aclements @rasky @josharian
The text was updated successfully, but these errors were encountered: