From 21cca30e5be80b5e7ca21904a6f8362cfd9456a1 Mon Sep 17 00:00:00 2001 From: Oleksandr Vodotiiets Date: Wed, 19 Aug 2026 21:01:20 +0300 Subject: [PATCH] perf: make orderedIDSet membership checks constant time (#5178) orderedIDSet.add scanned the entire existing slice for every inserted ID, so building a Collection was quadratic in the number of packages sharing an index key. This is most visible in idsByType: package types are few, so that index accumulates nearly every package in the SBOM into a single set, and each Add rescans it. Keep the ordered slice (ordering and dedup semantics are unchanged) and add a hash index for membership checks. The index is only built once a set grows beyond a small threshold, so the many single-element sets held by idsByName and idsByPath do not pay for a map, while the large idsByType sets get constant-time lookups. BenchmarkCollectionAdd (Apple M2 Pro), packages sharing one type: packages before after speedup 1,000 1.37 ms 0.76 ms 1.8x 10,000 100.31 ms 9.80 ms 10.2x 50,000 2,423.79 ms 34.46 ms 70.3x The tradeoff is memory: sets past the threshold allocate an index map, about 14% more bytes at 50,000 packages, with allocation counts unchanged. Signed-off-by: Oleksandr Vodotiiets <61548316+avodotiiets@users.noreply.github.com> --- syft/pkg/collection.go | 48 ++++++++++++++++++++++--- syft/pkg/collection_test.go | 71 +++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 5 deletions(-) diff --git a/syft/pkg/collection.go b/syft/pkg/collection.go index d6956c084..215e7eedf 100644 --- a/syft/pkg/collection.go +++ b/syft/pkg/collection.go @@ -299,19 +299,56 @@ func (c *Collection) Sorted(types ...Type) (pkgs []Package) { return pkgs } +// orderedIDSetIndexThreshold is the number of elements beyond which an orderedIDSet maintains a hash +// index for membership checks. Below this size a linear scan of the contiguous slice is competitive +// with a map lookup and avoids allocating a map for the many small (often single-element) sets held +// by the name and path indexes. Above it, the linear scan is what makes bulk insertion quadratic. +const orderedIDSetIndexThreshold = 16 + type orderedIDSet struct { slice []artifact.ID + + // index is nil until the set grows beyond orderedIDSetIndexThreshold, after which it is kept in + // sync with slice and used to answer membership checks in constant time. + index map[artifact.ID]struct{} +} + +func (s *orderedIDSet) contains(id artifact.ID) bool { + if s.index != nil { + _, exists := s.index[id] + return exists + } + + for _, existingID := range s.slice { + if existingID == id { + return true + } + } + + return false } func (s *orderedIDSet) add(ids ...artifact.ID) { -loopNewIDs: for _, newID := range ids { - for _, existingID := range s.slice { - if existingID == newID { - continue loopNewIDs - } + if s.contains(newID) { + continue } + s.slice = append(s.slice, newID) + + switch { + case s.index != nil: + s.index[newID] = struct{}{} + case len(s.slice) > orderedIDSetIndexThreshold: + s.buildIndex() + } + } +} + +func (s *orderedIDSet) buildIndex() { + s.index = make(map[artifact.ID]struct{}, len(s.slice)) + for _, id := range s.slice { + s.index[id] = struct{}{} } } @@ -319,6 +356,7 @@ func (s *orderedIDSet) delete(id artifact.ID) { for i, existingID := range s.slice { if existingID == id { s.slice = append(s.slice[:i], s.slice[i+1:]...) + delete(s.index, id) // no-op when the index has not been built return } } diff --git a/syft/pkg/collection_test.go b/syft/pkg/collection_test.go index 14f698c15..0d8c510ec 100644 --- a/syft/pkg/collection_test.go +++ b/syft/pkg/collection_test.go @@ -2,6 +2,7 @@ package pkg import ( "context" + "fmt" "testing" "github.com/scylladb/go-set/strset" @@ -482,3 +483,73 @@ func Test_idOrderedSet_add(t *testing.T) { }) } } + +func Test_idOrderedSet_addBeyondIndexThreshold(t *testing.T) { + // exercise the indexed path: sets larger than orderedIDSetIndexThreshold must still deduplicate + // and retain insertion ordering + const count = orderedIDSetIndexThreshold * 4 + + var expected []artifact.ID + var input []artifact.ID + for i := 0; i < count; i++ { + id := artifact.ID(fmt.Sprintf("id-%d", i)) + expected = append(expected, id) + input = append(input, id) + } + // add every element a second time; none of these should be appended + input = append(input, expected...) + + var s orderedIDSet + s.add(input...) + + assert.Equal(t, expected, s.slice) + require.NotNil(t, s.index, "index should be built once the set grows beyond the threshold") + assert.Len(t, s.index, len(expected), "index and slice should agree") +} + +func Test_idOrderedSet_deleteKeepsIndexInSync(t *testing.T) { + const count = orderedIDSetIndexThreshold * 2 + + var s orderedIDSet + for i := 0; i < count; i++ { + s.add(artifact.ID(fmt.Sprintf("id-%d", i))) + } + require.NotNil(t, s.index) + + removed := artifact.ID("id-0") + s.delete(removed) + + assert.Len(t, s.slice, count-1) + assert.NotContains(t, s.slice, removed) + assert.False(t, s.contains(removed), "deleted element must not remain in the index") + + // re-adding must work (it would be skipped if the index still held a stale entry) + s.add(removed) + assert.Equal(t, removed, s.slice[len(s.slice)-1]) +} + +func BenchmarkCollectionAdd(b *testing.B) { + // packages sharing a type all land in the same idsByType set, which is the case that previously + // made insertion quadratic + for _, count := range []int{1_000, 10_000, 50_000} { + pkgs := make([]Package, count) + for i := range pkgs { + pkgs[i] = Package{ + Name: fmt.Sprintf("package-%d", i), + Version: "1.0.0", + Type: NpmPkg, + } + pkgs[i].SetID() + } + + b.Run(fmt.Sprintf("packages=%d", count), func(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + c := NewCollection() + for _, p := range pkgs { + c.Add(p) + } + } + }) + } +}