diff --git a/internal/index/README.md b/internal/index/README.md index c2f378fce..93b6cbcb1 100644 --- a/internal/index/README.md +++ b/internal/index/README.md @@ -1,15 +1,14 @@ # internal/index A key-split (radix) index over strings, and a forward/reverse pair of them that answers prefix and -suffix lookups. Ported from a prototype directory-scan package and kept close to the original so the -two can be diffed. +suffix lookups. Ported from a prototype directory-scan package, minus its locking and JSON support: the +only consumer builds an index once and then only reads it, so it is not safe for concurrent writes. ## Types - `KeySplitIndex[T]` / `Node[T]`: a radix tree. Each node holds at most one value and splits its children on the longest common prefix of their keys, so `pom.xml` and `pom.properties` share a - `pom.` node. Reads take a read lock per node; writes upgrade to a write lock only where the tree - changes. `KeySplitIndex` also marshals to and from a flat JSON object of key → value. + `pom.` node. - `PrefixSuffix[T]`: two `KeySplitIndex` values, one keyed by the string and one by its reverse, so `ByPrefix("pom")` and `BySuffix(".jar")` are both a single descent followed by a collect. @@ -27,13 +26,11 @@ name repeats across directories. ## Where it is used -`internal/archive.Index` keeps one `PrefixSuffix[[]*node]` of files by base name. A cataloger glob +`internal/archive.Resolver` keeps one `PrefixSuffix[[]*node]` of files by base name, and one of directories. A cataloger glob whose last segment is a literal, `*suffix` or `prefix*` (nearly all of them: `**/*.jar`, `**/pom.properties`) is answered from it instead of by walking every path. ## Notes - Lookups are byte-wise; `reverse` works on runes so a multi-byte name reverses correctly. -- `lock.go` returns unlock functions rather than exposing `Unlock`, which is how a node tells a read - lock from a write lock when deciding whether to upgrade. - `_find`, `_collect` and `_makeNodeP` are the prototype's internals and are left with its naming. diff --git a/internal/index/key_split.go b/internal/index/key_split.go index a74378364..a24cfcb96 100644 --- a/internal/index/key_split.go +++ b/internal/index/key_split.go @@ -1,10 +1,9 @@ -// Package index provides a concurrent key-split (radix) index, and a forward/reverse pair over it -// that answers prefix and suffix lookups such as "which names end in .jar". +// Package index provides a key-split (radix) index, and a forward/reverse pair over it that answers +// prefix and suffix lookups such as "which names end in .jar". It is not safe for concurrent writes: the +// only consumer builds an index once and then only reads it. package index import ( - "bytes" - "encoding/json" "strings" ) @@ -14,51 +13,11 @@ type KeySplitIndex[T any] struct { Node[T] } -var _ interface { - json.Marshaler - json.Unmarshaler -} = (*KeySplitIndex[int])(nil) - -func (n *KeySplitIndex[T]) MarshalJSON() ([]byte, error) { - values := map[string]T{} - collectValues(values, "", &n.Node) - buf := &bytes.Buffer{} - enc := json.NewEncoder(buf) - enc.SetEscapeHTML(false) - err := enc.Encode(values) - if err != nil { - return nil, err - } - return buf.Bytes(), nil -} - -func (n *KeySplitIndex[T]) UnmarshalJSON(bytes []byte) error { - values := map[string]T{} - err := json.Unmarshal(bytes, &values) - if err != nil { - return err - } - for k, v := range values { - n.Set(k, v) - } - return nil -} - -func collectValues[T any](values map[string]T, key string, node *Node[T]) { - if node.set { - values[key] = node.value - } - for segment, child := range node.keyMap { - collectValues(values, key+segment, child) - } -} - // ----------------------- Node ----------------------- type NodeMap[T any] map[string]*Node[T] type Node[T any] struct { - lockable set bool value T keyMap NodeMap[T] @@ -67,11 +26,8 @@ type Node[T any] struct { type NodeUpdateFunc[T any] func(current *Node[T]) (newValue T) -func (n *Node[T]) Value() (v T) { - unlock := n.RLock() - v = n.value - unlock() - return +func (n *Node[T]) Value() T { + return n.value } func (n *Node[T]) Get(s string) (out T) { @@ -84,8 +40,7 @@ func (n *Node[T]) Get(s string) (out T) { } func (n *Node[T]) Set(name string, value T) { - node := n._makeNodeP(nil, name, nil) - node.SetValue(value) + n._makeNodeP(name, nil).SetValue(value) } func (n *Node[T]) ByPrefix(s string) []T { @@ -97,18 +52,13 @@ func (n *Node[T]) ByPrefix(s string) []T { } func (n *Node[T]) Update(name string, f NodeUpdateFunc[T]) { - unlock := n.Lock() - node := n._makeNodeP(&unlock, name, nil) - v := f(node) - node.SetValue(v) - unlock() + node := n._makeNodeP(name, nil) + node.SetValue(f(node)) } func (n *Node[T]) SetValue(value T) { - unlock := n.Lock() n.value = value n.set = true - unlock() } func (n *Node[T]) Collect() (values []T) { @@ -117,7 +67,6 @@ func (n *Node[T]) Collect() (values []T) { } func (n *Node[T]) _collect(values *[]T) { - unlock := n.RLock() if n.set { *values = append(*values, n.value) } @@ -125,14 +74,10 @@ func (n *Node[T]) _collect(values *[]T) { for _, v := range n.keyMap { v._collect(values) } - - unlock() } // _find returns the node for s and whether it matched exactly. func (n *Node[T]) _find(s string) (node *Node[T], equal bool) { - defer n.RLock()() - if s == "" { return n, true } @@ -170,139 +115,67 @@ func (n *Node[T]) _find(s string) (node *Node[T], equal bool) { return next._find(s[offset:]) } -//nolint:funlen,gocognit -func (n *Node[T]) _makeNodeP(unlock *func(), name string, nodeIfEmpty *Node[T]) *Node[T] { +// _makeNodeP returns the node for name, creating it (as nodeIfEmpty when given) and splitting existing +// keys on their longest common prefix as needed. +func (n *Node[T]) _makeNodeP(name string, nodeIfEmpty *Node[T]) *Node[T] { if name == "" { return n } - if unlock == nil { - rUnlock := n.RLock() - n = n._makeNodeP(&rUnlock, name, nodeIfEmpty) - rUnlock() - return n - } - ch := rune(name[0]) - - if n.keyByChar == nil || n.keyMap == nil { - n._exclusiveLock(unlock) - - if n.keyByChar == nil { - n.keyByChar = map[rune]string{} - } - if n.keyMap == nil { - n.keyMap = NodeMap[T]{} - } + if n.keyByChar == nil { + n.keyByChar = map[rune]string{} + n.keyMap = NodeMap[T]{} } key, ok := n.keyByChar[ch] - - // no entry for the given character - if !ok { - // check again with an exclusive lock before creating a new entry - n._exclusiveLock(unlock) - - key, ok = n.keyByChar[ch] - if !ok { - // no entry for the given character, create one; this is all we have to do - newNode := nodeIfEmpty - if newNode == nil { - newNode = &Node[T]{} - } - n.keyMap[name] = newNode - n.keyByChar[ch] = name - - return newNode - } - } - switch { - case key == name: - existingNode := n.keyMap[key] - if existingNode == nil { - // try this again, something changed between read checks and exclusive lock - return n._makeNodeP(unlock, name, nodeIfEmpty) - } - - return existingNode - case strings.HasPrefix(key, name): - // existing key is longer than my key, we can just use the existing and make a new sub-entry for the longer key - n._exclusiveLock(unlock) - - existingNode := n.keyMap[key] - if existingNode == nil { - // try this again, something changed between read checks and exclusive lock - return n._makeNodeP(unlock, name, nodeIfEmpty) - } - delete(n.keyMap, key) - - newNode := nodeIfEmpty - if newNode == nil { - newNode = &Node[T]{} - } + case !ok: + // no entry for the given character, create one; this is all we have to do + newNode := orNewNode(nodeIfEmpty) n.keyMap[name] = newNode n.keyByChar[ch] = name - - next := key[len(name):] - _ = newNode._makeNodeP(nil, next, existingNode) - + return newNode + case key == name: + return n.keyMap[key] + case strings.HasPrefix(key, name): + // existing key is longer than my key, we can just use the existing and make a new sub-entry for the longer key + existingNode := n.keyMap[key] + delete(n.keyMap, key) + newNode := orNewNode(nodeIfEmpty) + n.keyMap[name] = newNode + n.keyByChar[ch] = name + newNode._makeNodeP(key[len(name):], existingNode) return newNode case strings.HasPrefix(name, key): // existing key is shorter than my key, we can just take the substring to remove the // existing string prefix and set the new node as a child of the existing node - n._exclusiveLock(unlock) - - existingNode := n.keyMap[key] - if existingNode == nil { - // try this again, something changed between read checks and exclusive lock - return n._makeNodeP(unlock, name, nodeIfEmpty) - } - - next := name[len(key):] - return existingNode._makeNodeP(nil, next, nodeIfEmpty) + return n.keyMap[key]._makeNodeP(name[len(key):], nodeIfEmpty) default: // neither the existing key nor my key contains a prefix of the other, so we find // the longest common prefix and split BOTH entries as children of a new entry with this common prefix - n._exclusiveLock(unlock) - commonLength := 1 // the first character already matches - minLength := len(key) - if len(name) < minLength { - minLength = len(name) - } - for ; commonLength < minLength; commonLength++ { - if key[commonLength] != name[commonLength] { - break - } + for commonLength < min(len(key), len(name)) && key[commonLength] == name[commonLength] { + commonLength++ } existingNode := n.keyMap[key] - if existingNode == nil { - // try this again, something changed between read checks and exclusive lock - return n._makeNodeP(unlock, name, nodeIfEmpty) - } delete(n.keyMap, key) - - newNode := nodeIfEmpty - if newNode == nil { - newNode = &Node[T]{} - } + newNode := orNewNode(nodeIfEmpty) parentNode := &Node[T]{} - parentNode._makeNodeP(nil, key[commonLength:], existingNode) - parentNode._makeNodeP(nil, name[commonLength:], newNode) + parentNode._makeNodeP(key[commonLength:], existingNode) + parentNode._makeNodeP(name[commonLength:], newNode) common := key[:commonLength] n.keyMap[common] = parentNode n.keyByChar[ch] = common - return newNode } } -func (n *Node[T]) _exclusiveLock(unlocker *func()) { - if !n.isExclusiveLock(*unlocker) { - (*unlocker)() - *unlocker = n.Lock() +func orNewNode[T any](n *Node[T]) *Node[T] { + if n == nil { + return &Node[T]{} } + return n } diff --git a/internal/index/key_split_test.go b/internal/index/key_split_test.go index 321d4d4f6..33d931f68 100644 --- a/internal/index/key_split_test.go +++ b/internal/index/key_split_test.go @@ -1,7 +1,6 @@ package index import ( - "encoding/json" "fmt" "testing" @@ -131,33 +130,6 @@ func Test_KeySplitIndex_Get(t *testing.T) { require.Equal(t, 0, fi.Get("invalid")) // returns the zero value } -func Test_KeySplitIndex_serialization(t *testing.T) { - index1 := KeySplitIndex[int]{} - - const ( - one = 1 - two = 2 - once = 11 - onesie = 111 - ) - - index1.Set("one", one) - index1.Set("two", two) - index1.Set("once", once) - index1.Set("onesie", onesie) - - serialized, err := json.Marshal(&index1) - require.NoError(t, err) - - require.JSONEq(t, `{"one":1,"two":2,"once":11,"onesie":111}`, string(serialized)) - - var index2 KeySplitIndex[int] - err = json.Unmarshal(serialized, &index2) - require.NoError(t, err) - - require.Equal(t, &index1, &index2) -} - // requireHasAll asserts the values are exactly the required ones, in any order. Carried over with the // index's own tests so they run here unchanged. func requireHasAll[T comparable](t *testing.T, values []T, required ...T) { diff --git a/internal/index/lock.go b/internal/index/lock.go deleted file mode 100644 index e399e9b0f..000000000 --- a/internal/index/lock.go +++ /dev/null @@ -1,30 +0,0 @@ -package index - -import ( - "reflect" - "sync" -) - -// lockable is a read-write lock that returns its own unlock function. -// -// Handing back an opaque unlocker (rather than exposing Unlock) lets a node tell whether the caller -// holds the read or write lock and upgrade only when needed. Carried over from the prototype index. -type lockable struct { - lock sync.RWMutex -} - -func (l *lockable) Lock() (unlock func()) { - l.lock.Lock() - return l.lock.Unlock -} - -func (l *lockable) RLock() (unlock func()) { - l.lock.RLock() - return l.lock.RUnlock -} - -// isExclusiveLock reports whether the given unlocker is the write lock's rather than the read -// lock's. Compared by function pointer because that is the only handle the caller was given. -func (l *lockable) isExclusiveLock(fn func()) bool { - return reflect.ValueOf(l.lock.Unlock).Pointer() == reflect.ValueOf(fn).Pointer() -}