mirror of https://github.com/tailscale/tailscale/
util/deephash: improve cycle detection (#2470)
The previous algorithm used a map of all visited pointers. The strength of this approach is that it quickly prunes any nodes that we have ever visited before. The detriment of the approach is that pruning is heavily dependent on the order that pointers were visited. This is especially relevant for hashing a map where map entries are visited in a non-deterministic manner, which would cause the map hash to be non-deterministic (which defeats the point of a hash). This new algorithm uses a stack of all visited pointers, similar to how github.com/google/go-cmp performs cycle detection. When we visit a pointer, we push it onto the stack, and when we leave a pointer, we pop it from the stack. Before visiting a pointer, we first check whether the pointer exists anywhere in the stack. If yes, then we prune the node. The detriment of this approach is that we may hash a node more often than before since we do not prune as aggressively. The set of visited pointers up until any node is only the path of nodes up to that node and not any other pointers that may have been visited elsewhere. This provides us deterministic hashing regardless of visit order. We can now delete hashMapFallback and associated complexity, which only exists because the previous approach was non-deterministic in the presence of cycles. This fixes a failure of the old algorithm where obviously different values are treated as equal because the pruning was too aggresive. See https://github.com/tailscale/tailscale/issues/2443#issuecomment-883653534 The new algorithm is slightly slower since it prunes less aggresively: name old time/op new time/op delta Hash-8 66.1µs ± 1% 68.8µs ± 1% +4.09% (p=0.000 n=19+19) HashMapAcyclic-8 63.0µs ± 1% 62.5µs ± 1% -0.76% (p=0.000 n=18+19) TailcfgNode-8 9.79µs ± 2% 9.88µs ± 1% +0.95% (p=0.000 n=19+17) HashArray-8 643ns ± 1% 653ns ± 1% +1.64% (p=0.000 n=19+19) However, a slower but more correct algorithm seems more favorable than a faster but incorrect algorithm. Signed-off-by: Joe Tsai <joetsai@digital-static.net>pull/2503/head
Joe Tsai
3 years ago
committed by
GitHub
parent
7b295f3d21
commit
d145c594ad
@ -1,224 +0,0 @@
|
||||
// Copyright (c) 2020 Tailscale Inc & AUTHORS All rights reserved.
|
||||
// Use of this source code is governed by a BSD-style
|
||||
// license that can be found in the LICENSE file.
|
||||
|
||||
// and
|
||||
|
||||
// Copyright 2018 The Go Authors. All rights reserved.
|
||||
// Use of this source code is governed by a BSD-style
|
||||
// license that can be found in the LICENSE file.
|
||||
|
||||
// This is a slightly modified fork of Go's src/internal/fmtsort/sort.go
|
||||
|
||||
package deephash
|
||||
|
||||
import (
|
||||
"reflect"
|
||||
"sort"
|
||||
)
|
||||
|
||||
// Note: Throughout this package we avoid calling reflect.Value.Interface as
|
||||
// it is not always legal to do so and it's easier to avoid the issue than to face it.
|
||||
|
||||
// sortedMap represents a map's keys and values. The keys and values are
|
||||
// aligned in index order: Value[i] is the value in the map corresponding to Key[i].
|
||||
type sortedMap struct {
|
||||
Key []reflect.Value
|
||||
Value []reflect.Value
|
||||
}
|
||||
|
||||
func (o *sortedMap) Len() int { return len(o.Key) }
|
||||
func (o *sortedMap) Less(i, j int) bool { return compare(o.Key[i], o.Key[j]) < 0 }
|
||||
func (o *sortedMap) Swap(i, j int) {
|
||||
o.Key[i], o.Key[j] = o.Key[j], o.Key[i]
|
||||
o.Value[i], o.Value[j] = o.Value[j], o.Value[i]
|
||||
}
|
||||
|
||||
// Sort accepts a map and returns a sortedMap that has the same keys and
|
||||
// values but in a stable sorted order according to the keys, modulo issues
|
||||
// raised by unorderable key values such as NaNs.
|
||||
//
|
||||
// The ordering rules are more general than with Go's < operator:
|
||||
//
|
||||
// - when applicable, nil compares low
|
||||
// - ints, floats, and strings order by <
|
||||
// - NaN compares less than non-NaN floats
|
||||
// - bool compares false before true
|
||||
// - complex compares real, then imag
|
||||
// - pointers compare by machine address
|
||||
// - channel values compare by machine address
|
||||
// - structs compare each field in turn
|
||||
// - arrays compare each element in turn.
|
||||
// Otherwise identical arrays compare by length.
|
||||
// - interface values compare first by reflect.Type describing the concrete type
|
||||
// and then by concrete value as described in the previous rules.
|
||||
//
|
||||
func newSortedMap(mapValue reflect.Value) *sortedMap {
|
||||
if mapValue.Type().Kind() != reflect.Map {
|
||||
return nil
|
||||
}
|
||||
// Note: this code is arranged to not panic even in the presence
|
||||
// of a concurrent map update. The runtime is responsible for
|
||||
// yelling loudly if that happens. See issue 33275.
|
||||
n := mapValue.Len()
|
||||
key := make([]reflect.Value, 0, n)
|
||||
value := make([]reflect.Value, 0, n)
|
||||
iter := mapValue.MapRange()
|
||||
for iter.Next() {
|
||||
key = append(key, iter.Key())
|
||||
value = append(value, iter.Value())
|
||||
}
|
||||
sorted := &sortedMap{
|
||||
Key: key,
|
||||
Value: value,
|
||||
}
|
||||
sort.Stable(sorted)
|
||||
return sorted
|
||||
}
|
||||
|
||||
// compare compares two values of the same type. It returns -1, 0, 1
|
||||
// according to whether a > b (1), a == b (0), or a < b (-1).
|
||||
// If the types differ, it returns -1.
|
||||
// See the comment on Sort for the comparison rules.
|
||||
func compare(aVal, bVal reflect.Value) int {
|
||||
aType, bType := aVal.Type(), bVal.Type()
|
||||
if aType != bType {
|
||||
return -1 // No good answer possible, but don't return 0: they're not equal.
|
||||
}
|
||||
switch aVal.Kind() {
|
||||
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
|
||||
a, b := aVal.Int(), bVal.Int()
|
||||
switch {
|
||||
case a < b:
|
||||
return -1
|
||||
case a > b:
|
||||
return 1
|
||||
default:
|
||||
return 0
|
||||
}
|
||||
case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
|
||||
a, b := aVal.Uint(), bVal.Uint()
|
||||
switch {
|
||||
case a < b:
|
||||
return -1
|
||||
case a > b:
|
||||
return 1
|
||||
default:
|
||||
return 0
|
||||
}
|
||||
case reflect.String:
|
||||
a, b := aVal.String(), bVal.String()
|
||||
switch {
|
||||
case a < b:
|
||||
return -1
|
||||
case a > b:
|
||||
return 1
|
||||
default:
|
||||
return 0
|
||||
}
|
||||
case reflect.Float32, reflect.Float64:
|
||||
return floatCompare(aVal.Float(), bVal.Float())
|
||||
case reflect.Complex64, reflect.Complex128:
|
||||
a, b := aVal.Complex(), bVal.Complex()
|
||||
if c := floatCompare(real(a), real(b)); c != 0 {
|
||||
return c
|
||||
}
|
||||
return floatCompare(imag(a), imag(b))
|
||||
case reflect.Bool:
|
||||
a, b := aVal.Bool(), bVal.Bool()
|
||||
switch {
|
||||
case a == b:
|
||||
return 0
|
||||
case a:
|
||||
return 1
|
||||
default:
|
||||
return -1
|
||||
}
|
||||
case reflect.Ptr:
|
||||
a, b := aVal.Pointer(), bVal.Pointer()
|
||||
switch {
|
||||
case a < b:
|
||||
return -1
|
||||
case a > b:
|
||||
return 1
|
||||
default:
|
||||
return 0
|
||||
}
|
||||
case reflect.Chan:
|
||||
if c, ok := nilCompare(aVal, bVal); ok {
|
||||
return c
|
||||
}
|
||||
ap, bp := aVal.Pointer(), bVal.Pointer()
|
||||
switch {
|
||||
case ap < bp:
|
||||
return -1
|
||||
case ap > bp:
|
||||
return 1
|
||||
default:
|
||||
return 0
|
||||
}
|
||||
case reflect.Struct:
|
||||
for i := 0; i < aVal.NumField(); i++ {
|
||||
if c := compare(aVal.Field(i), bVal.Field(i)); c != 0 {
|
||||
return c
|
||||
}
|
||||
}
|
||||
return 0
|
||||
case reflect.Array:
|
||||
for i := 0; i < aVal.Len(); i++ {
|
||||
if c := compare(aVal.Index(i), bVal.Index(i)); c != 0 {
|
||||
return c
|
||||
}
|
||||
}
|
||||
return 0
|
||||
case reflect.Interface:
|
||||
if c, ok := nilCompare(aVal, bVal); ok {
|
||||
return c
|
||||
}
|
||||
c := compare(reflect.ValueOf(aVal.Elem().Type()), reflect.ValueOf(bVal.Elem().Type()))
|
||||
if c != 0 {
|
||||
return c
|
||||
}
|
||||
return compare(aVal.Elem(), bVal.Elem())
|
||||
default:
|
||||
// Certain types cannot appear as keys (maps, funcs, slices), but be explicit.
|
||||
panic("bad type in compare: " + aType.String())
|
||||
}
|
||||
}
|
||||
|
||||
// nilCompare checks whether either value is nil. If not, the boolean is false.
|
||||
// If either value is nil, the boolean is true and the integer is the comparison
|
||||
// value. The comparison is defined to be 0 if both are nil, otherwise the one
|
||||
// nil value compares low. Both arguments must represent a chan, func,
|
||||
// interface, map, pointer, or slice.
|
||||
func nilCompare(aVal, bVal reflect.Value) (int, bool) {
|
||||
if aVal.IsNil() {
|
||||
if bVal.IsNil() {
|
||||
return 0, true
|
||||
}
|
||||
return -1, true
|
||||
}
|
||||
if bVal.IsNil() {
|
||||
return 1, true
|
||||
}
|
||||
return 0, false
|
||||
}
|
||||
|
||||
// floatCompare compares two floating-point values. NaNs compare low.
|
||||
func floatCompare(a, b float64) int {
|
||||
switch {
|
||||
case isNaN(a):
|
||||
return -1 // No good answer if b is a NaN so don't bother checking.
|
||||
case isNaN(b):
|
||||
return 1
|
||||
case a < b:
|
||||
return -1
|
||||
case a > b:
|
||||
return 1
|
||||
}
|
||||
return 0
|
||||
}
|
||||
|
||||
func isNaN(a float64) bool {
|
||||
return a != a
|
||||
}
|
||||
Loading…
Reference in New Issue