msgpack: optimize

[zchee]

Optimize msgpack package.

[codecov]

Codecov Report

Merging #95 (2732b5d) into master (7c85d51) will decrease coverage by 0.0%.
The diff coverage is 91.4%.

@@           Coverage Diff            @@
##           master     #95     +/-   ##
========================================
- Coverage    80.6%   80.6%   -0.1%     
========================================
  Files          14      14             
  Lines        3044    3048      +4     
========================================
+ Hits         2456    2457      +1     
- Misses        588     591      +3     

Flag	Coverage Δ
linux-1.15-nightly	80.5% <91.4%> (-0.1%)	⬇️
linux-1.15-stable	79.8% <91.4%> (-0.1%)	⬇️
linux-1.16-nightly	80.5% <91.4%> (-0.1%)	⬇️
linux-1.16-stable	79.8% <91.4%> (-0.1%)	⬇️
linux-1.17-nightly	80.5% <91.4%> (-0.1%)	⬇️
linux-1.17-stable	79.8% <91.4%> (-0.1%)	⬇️
macos-1.15-nightly	80.5% <91.4%> (-0.1%)	⬇️
macos-1.15-stable	79.8% <91.4%> (-0.1%)	⬇️
macos-1.16-nightly	80.5% <91.4%> (-0.1%)	⬇️
macos-1.16-stable	79.8% <91.4%> (-0.1%)	⬇️
macos-1.17-nightly	80.5% <91.4%> (-0.1%)	⬇️
macos-1.17-stable	79.8% <91.4%> (-0.1%)	⬇️
windows-1.15-nightly	79.7% <91.4%> (-0.1%)	⬇️
windows-1.15-stable	78.9% <91.4%> (-0.1%)	⬇️
windows-1.16-nightly	79.7% <91.4%> (-0.1%)	⬇️
windows-1.16-stable	78.9% <91.4%> (-0.1%)	⬇️
windows-1.17-nightly	79.7% <91.4%> (-0.1%)	⬇️
windows-1.17-stable	78.9% <91.4%> (-0.1%)	⬇️

Impacted Files	Coverage Δ
msgpack/field.go	76.5% <70.0%> (-1.9%)	⬇️
msgpack/unpack.go	94.3% <100.0%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c85d51...2732b5d. Read the comment docs.

[nieomylnieja]

Can you elaborate on that? I've seen bounds checks (namely the std lib) to guard a specific length of a slice, but not ever like this

[zchee]

This commit is very old so I don't remember that, but IIRC scope is for i := 0; i < len(fields); i++.

[nieomylnieja]

yeah, but when do you expect len(fields) to go out of bounds? Usually I've seen bound checks like these guard a slice of unknown length when we're explicitly accessing certain indexes like this:

func someFunc(s []string) {
  _ = s[1]
  s[0] = "hey"
  s[1] = "test!"
}

While what you've done here is you're not guarding an explicit length like: _ = fields[3] but guarding the size which is restricted by the language itself, that is: the last index has to be len(slice)-1, there's no other way. It seems completely redundant to me and confusing.

Am I missing something? :)

[zchee]

I'll re-detect where is BCE optimized using -gcflags='-d=ssa/check_bce/debug=3'.

But in context of language specifications, the fields value is not array, actual type is slice. It means, the gc compiler (still can't optimization? I don't know) might be imagine will growing slice. And actually we also can grow fields slice after the BCE'ed place.

e.g., encoding/binary package is return byte slice immediately after the BCE place, but this msgpack packages logic is used for loop (and etc) after the BCE place, therefore works BCE optimization. (above is my current imagine. I'll verify again that the BCE is working properly when I have time anyway.)

[nieomylnieja]

interesting!

Still, fields is not local to the collect function and might be manipulated after you do the bounds check so you won't really eliminate the BCE done on fields[j] = fields[i], I've verified that with this whole file:

• before manual bounds check:

$ go run -gcflags="-d=ssa/check_bce/debug=1" field.go
# command-line-arguments
./field.go:83:12: Found IsInBounds
./field.go:87:19: Found IsSliceInBounds
./field.go:100:10: Found IsInBounds

• after:

└> go run -gcflags="-d=ssa/check_bce/debug=1" field.go
# command-line-arguments
./field.go:78:17: Found IsInBounds
./field.go:84:12: Found IsInBounds
./field.go:88:19: Found IsSliceInBounds
./field.go:101:10: Found IsInBounds

You can see you've just introduced an extra check there and haven't got rid of the ones performed in the loop.

Correct me If I'm wrong and thanks a lot for answering, this is quiet interesting! :)