Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upcmd/compile: unexpected prove/BCE regressions when building encoding/json #31275
Comments
This comment has been minimized.
This comment has been minimized.
Are you able to run a bisect? |
This comment has been minimized.
This comment has been minimized.
Bisected: 83a33d3 is the first bad commit |
This comment has been minimized.
This comment has been minimized.
/cc @randall77 as FYI |
This comment has been minimized.
This comment has been minimized.
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. |
This comment has been minimized.
This comment has been minimized.
I fail to understand how the current code manages to check that j or k are non negative. @randall77? |
This comment has been minimized.
This comment has been minimized.
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. |
This comment has been minimized.
This comment has been minimized.
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 |
This comment has been minimized.
This comment has been minimized.
@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 |
This comment has been minimized.
This comment has been minimized.
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: |
This comment has been minimized.
This comment has been minimized.
@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: |
This comment has been minimized.
This comment has been minimized.
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. |
This comment has been minimized.
This comment has been minimized.
Right. It's partly just a matter of accounting: we're removing a |
This comment has been minimized.
This comment has been minimized.
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? |
This comment has been minimized.
This comment has been minimized.
IsSliceInBounds is a single instruction. For
Exactly.
Yes. |
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