msgpack: optimize #95
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
zchee
force-pushed
the
msgpack/optimize
branch
2 times, most recently
from
cb7cb3d to
710d3d8
Compare
zchee
force-pushed
the
msgpack/optimize
branch
from
710d3d8 to
722b71d
Compare
zchee
force-pushed
the
msgpack/optimize
branch
from
722b71d to
e45da9e
Compare
zchee
force-pushed
the
msgpack/optimize
branch
from
e45da9e to
fd4c685
Compare
zchee
force-pushed
the
msgpack/optimize
branch
from
fd4c685 to
b728b30
Compare
Signed-off-by: Koichi Shiraishi <[email protected]>
Signed-off-by: Koichi Shiraishi <[email protected]>
Signed-off-by: Koichi Shiraishi <[email protected]>
Signed-off-by: Koichi Shiraishi <[email protected]>
Signed-off-by: Koichi Shiraishi <[email protected]>
name old time/op new time/op delta _correctFilels/Named-20 9.55µs ± 0% 9.45µs ± 1% -1.02% (p=0.008 n=5+5) _correctFilels/Empty-20 9.60µs ± 0% 9.88µs ±10% ~ (p=1.000 n=5+5) _correctFilels/OmitEmpty-20 10.9µs ± 1% 11.2µs ± 6% ~ (p=0.095 n=5+5) [Geo mean] 10.0µs 10.1µs +1.38% name old alloc/op new alloc/op delta _correctFilels/Named-20 4.75kB ± 0% 4.75kB ± 0% ~ (all equal) _correctFilels/Empty-20 4.75kB ± 0% 4.75kB ± 0% ~ (p=0.333 n=5+4) _correctFilels/OmitEmpty-20 5.10kB ± 0% 5.10kB ± 0% ~ (all equal) [Geo mean] 4.86kB 4.86kB -0.00% name old allocs/op new allocs/op delta _correctFilels/Named-20 96.0 ± 0% 96.0 ± 0% ~ (all equal) _correctFilels/Empty-20 96.0 ± 0% 96.0 ± 0% ~ (all equal) _correctFilels/OmitEmpty-20 96.0 ± 0% 96.0 ± 0% ~ (all equal) [Geo mean] 96.0 96.0 +0.00% Signed-off-by: Koichi Shiraishi <[email protected]>
zchee
force-pushed
the
msgpack/optimize
branch
from
b728b30 to
2732b5d
Compare
| @@ -74,6 +74,8 @@ func collectFields(fields []*field, t reflect.Type, visited map[reflect.Type]boo | |||
| } | |||
|
|
|||
| if len(index) == d { | |||
| _ = fields[len(fields)-1] // bounds check hint to compiler | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
This commit is very old so I don't remember that, but IIRC scope is for i := 0; i < len(fields); i++.
There was a problem hiding this comment.
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? :)
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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 IsInBoundsYou 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! :)
Optimize
msgpackpackage.