From 8e2cdf2429808c1c033af306c386ddf2939cec09 Mon Sep 17 00:00:00 2001 From: Alex Goodman Date: Thu, 24 Sep 2026 13:42:59 -0400 Subject: [PATCH] chore: drop the locking and JSON support from internal/index the index was ported from a prototype with per-node `RWMutex` lock upgrading and JSON marshalling. Its only consumer (`archive.Resolver`) builds it sequentially during extraction and then only reads it, so neither was used. `lock.go` also compared method-value pointers to tell the read lock from the write lock, which can't distinguish lock instances. removed the locks, the upgrade and retry paths that existed only for concurrent writers, and the JSON methods and their test. The radix logic is unchanged, and the README now points at `archive.Resolver` rather than a nonexistent `archive.Index`. Signed-off-by: Alex Goodman --- internal/index/README.md | 11 +- internal/index/key_split.go | 203 ++++++------------------------- internal/index/key_split_test.go | 28 ----- internal/index/lock.go | 30 ----- 4 files changed, 42 insertions(+), 230 deletions(-) delete mode 100644 internal/index/lock.go 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() -}