From 825d85458fa24de37fea4c1b3c45e6afc8bb988e Mon Sep 17 00:00:00 2001 From: Thomas Date: Sun, 1 Nov 2026 21:55:31 +1100 Subject: [PATCH] perf(index): stop tracking the files being indexed AddIndexingFile copies the whole set on every call, which is quadratic in how deeply external files nest. Nothing in the library has read the set since the lookup in find_component_external.go was removed, so stop building it and deprecate the exported helpers. --- index/rolodex_file_loader.go | 6 +- index/rolodex_nested_files_test.go | 103 +++++++++++++++++++++++++++++ index/rolodex_owned_file.go | 3 +- index/rolodex_remote_loader.go | 10 +-- index/search_index.go | 19 ++++-- index/search_index_test.go | 10 +++ 6 files changed, 133 insertions(+), 18 deletions(-) create mode 100644 index/rolodex_nested_files_test.go diff --git a/index/rolodex_file_loader.go b/index/rolodex_file_loader.go index 63e5cb1b..abfa63e4 100644 --- a/index/rolodex_file_loader.go +++ b/index/rolodex_file_loader.go @@ -157,11 +157,7 @@ func (l *LocalFS) OpenWithContext(ctx context.Context, name string) (fs.File, er copiedCfg.Rolodex = l.rolodex } - // Add this file to the context's indexing set to prevent deadlocks - // when circular references cause the same file to be looked up recursively. - indexingCtx := AddIndexingFile(ctx, name) - - idx, _ := extractedFile.IndexWithContext(indexingCtx, &copiedCfg) + idx, _ := extractedFile.IndexWithContext(ctx, &copiedCfg) if idx != nil { NewResolver(idx) diff --git a/index/rolodex_nested_files_test.go b/index/rolodex_nested_files_test.go new file mode 100644 index 00000000..5bb2324e --- /dev/null +++ b/index/rolodex_nested_files_test.go @@ -0,0 +1,103 @@ +package index + +import ( + "context" + "fmt" + "io" + "net/http" + "strconv" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/pb33f/go-yaml" + "github.com/pb33f/testify/assert" + "github.com/pb33f/testify/require" +) + +const nestedRemoteFilesBase = "https://nested.test/" + +func nestedRemoteFileRef(n int) string { + return fmt.Sprintf("components:\n schemas:\n A:\n $ref: '%sf%d.yaml#/components/schemas/A'\n", nestedRemoteFilesBase, n) +} + +// indexNestedRemoteFiles indexes a root document followed by a chain of remote files f1 to +// fDepth, where each file references the next, so each one is indexed while the ones before +// it still are. It returns the number of files fetched and the result of indexing. +func indexNestedRemoteFiles(depth int, lastFile string) (int64, error) { + var fetched atomic.Int64 + cfg := CreateOpenAPIIndexConfig() + cfg.RemoteURLHandler = func(url string) (*http.Response, error) { + name := strings.TrimSuffix(strings.TrimPrefix(url, nestedRemoteFilesBase+"f"), ".yaml") + n, err := strconv.Atoi(name) + if err != nil { + return nil, fmt.Errorf("unexpected url %q", url) + } + fetched.Add(1) + doc := lastFile + if n < depth { + doc = nestedRemoteFileRef(n + 1) + } + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(doc)), + }, nil + } + + var root yaml.Node + if err := yaml.Unmarshal([]byte(nestedRemoteFileRef(1)), &root); err != nil { + return 0, err + } + + rolodex := NewRolodex(cfg) + remoteFS, err := NewRemoteFSWithConfig(cfg) + if err != nil { + return 0, err + } + rolodex.AddRemoteFS(nestedRemoteFilesBase, remoteFS) + rolodex.SetRootNode(&root) + err = rolodex.IndexTheRolodex(context.Background()) + return fetched.Load(), err +} + +const nestedRemoteLeaf = "components:\n schemas:\n A:\n type: string\n" + +func TestRolodex_NestedRemoteFiles_IndexesEveryFile(t *testing.T) { + fetched, err := indexNestedRemoteFiles(50, nestedRemoteLeaf) + require.NoError(t, err) + assert.Equal(t, int64(50), fetched) +} + +func TestRolodex_CircularRemoteFiles_DoNotDeadlock(t *testing.T) { + done := make(chan int64, 1) + go func() { + // The result of indexing depends on circular reference handling; only finishing matters here. + fetched, _ := indexNestedRemoteFiles(3, nestedRemoteFileRef(1)) + done <- fetched + }() + + select { + case fetched := <-done: + assert.Equal(t, int64(3), fetched) + case <-time.After(30 * time.Second): + t.Fatal("indexing circular remote files did not finish") + } +} + +func BenchmarkRolodex_NestedRemoteFiles(b *testing.B) { + for _, depth := range []int{10, 100, 500} { + b.Run(fmt.Sprintf("depth=%d", depth), func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + fetched, err := indexNestedRemoteFiles(depth, nestedRemoteLeaf) + if err != nil { + b.Fatal(err) + } + if fetched != int64(depth) { + b.Fatalf("fetched %d files, want %d", fetched, depth) + } + } + }) + } +} diff --git a/index/rolodex_owned_file.go b/index/rolodex_owned_file.go index 247e8d7c..cfcead4b 100644 --- a/index/rolodex_owned_file.go +++ b/index/rolodex_owned_file.go @@ -54,8 +54,7 @@ func (r *Rolodex) ownIndexedFile(source RolodexFile) (*LocalFile, error) { cfg.SpecAbsolutePath = location cfg.SpecInfo = info cfg.AvoidBuildIndex = true - ctx := AddIndexingFile(context.Background(), location) - idx := NewSpecIndexWithConfigAndContext(ctx, info.RootNode, &cfg) + idx := NewSpecIndexWithConfigAndContext(context.Background(), info.RootNode, &cfg) resolver := NewResolver(idx) resolver.IgnoreArray = cfg.IgnoreArrayCircularReferences resolver.IgnorePoly = cfg.IgnorePolymorphicCircularReferences diff --git a/index/rolodex_remote_loader.go b/index/rolodex_remote_loader.go index aa8e89e2..57699bd4 100644 --- a/index/rolodex_remote_loader.go +++ b/index/rolodex_remote_loader.go @@ -644,7 +644,7 @@ func (i *RemoteFS) OpenWithContext(ctx context.Context, remoteURL string) (fs.Fi i.Files.Store(cacheKey, remoteFile) i.releaseRemoteProcessingWaiter(processingWaiter, cacheKey, remoteFile, nil) - i.indexRemoteFile(ctx, remoteFile, copiedCfg, remoteParsedURL, remoteParsedURLOriginal) + i.indexRemoteFile(ctx, remoteFile, copiedCfg, remoteParsedURL) return remoteFile, errors.Join(i.remoteErrors...) } @@ -800,13 +800,9 @@ func (i *RemoteFS) indexRemoteFile( ctx context.Context, remoteFile *RemoteFile, copiedCfg *SpecIndexConfig, - remoteParsedURL, remoteParsedURLOriginal *url.URL, + remoteParsedURL *url.URL, ) { - indexingCtx := AddIndexingFile(ctx, remoteParsedURL.Path) - indexingCtx = AddIndexingFile(indexingCtx, remoteParsedURL.String()) - indexingCtx = AddIndexingFile(indexingCtx, remoteParsedURLOriginal.String()) - - idx, idxError := remoteFile.Index(indexingCtx, copiedCfg) + idx, idxError := remoteFile.Index(ctx, copiedCfg) if idxError != nil && idx == nil { i.appendRemoteError(idxError) remoteFile.signalIndexingComplete() diff --git a/index/search_index.go b/index/search_index.go index d61e6a85..19b7b9f8 100644 --- a/index/search_index.go +++ b/index/search_index.go @@ -14,14 +14,20 @@ import ( type ContextKey string const ( - CurrentPathKey ContextKey = "currentPath" - FoundIndexKey ContextKey = "foundIndex" - RootIndexKey ContextKey = "currentIndex" - IndexingFilesKey ContextKey = "indexingFiles" // Tracks files being indexed in current call chain + CurrentPathKey ContextKey = "currentPath" + FoundIndexKey ContextKey = "foundIndex" + RootIndexKey ContextKey = "currentIndex" + // IndexingFilesKey holds the set used by AddIndexingFile and GetIndexingFiles. + // + // Deprecated: the library no longer tracks the files it is indexing. + IndexingFilesKey ContextKey = "indexingFiles" ) // GetIndexingFiles returns the set of files currently being indexed in the call chain. // Returns nil if not set. +// +// Deprecated: the library no longer tracks the files it is indexing, so this only +// returns what the caller added with AddIndexingFile. func GetIndexingFiles(ctx context.Context) map[string]bool { if v := ctx.Value(IndexingFilesKey); v != nil { return v.(map[string]bool) @@ -31,6 +37,8 @@ func GetIndexingFiles(ctx context.Context) map[string]bool { // AddIndexingFile adds a file to the indexing set in the context. // Returns a new context with the updated set. +// +// Deprecated: the library no longer reads this set when indexing. func AddIndexingFile(ctx context.Context, filePath string) context.Context { existing := GetIndexingFiles(ctx) newSet := make(map[string]bool) @@ -45,6 +53,9 @@ func AddIndexingFile(ctx context.Context, filePath string) context.Context { // For HTTP URLs, it also checks if the PATH portion matches any indexed file, // since the same file might be referenced with different hostnames (which get normalized // to a common server). +// +// Deprecated: the library no longer tracks the files it is indexing, so this only +// sees what the caller added with AddIndexingFile. func IsFileBeingIndexed(ctx context.Context, filePath string) bool { files := GetIndexingFiles(ctx) if files == nil { diff --git a/index/search_index_test.go b/index/search_index_test.go index 865738e5..4cc1e608 100644 --- a/index/search_index_test.go +++ b/index/search_index_test.go @@ -832,6 +832,16 @@ func TestIsFileBeingIndexed_HTTPPathMatch(t *testing.T) { assert.False(t, IsFileBeingIndexed(ctx, "https://example.com/other/file.yaml")) } +func TestAddIndexingFile_ExtendsTheSetOfTheChildContextOnly(t *testing.T) { + parent := AddIndexingFile(context.Background(), "/tmp/specs/pet.yaml") + child := AddIndexingFile(parent, "/tmp/specs/cat.yaml") + + assert.Equal(t, map[string]bool{"/tmp/specs/pet.yaml": true}, GetIndexingFiles(parent)) + assert.Equal(t, map[string]bool{"/tmp/specs/pet.yaml": true, "/tmp/specs/cat.yaml": true}, GetIndexingFiles(child)) + assert.True(t, IsFileBeingIndexed(child, "/tmp/specs/cat.yaml")) + assert.False(t, IsFileBeingIndexed(parent, "/tmp/specs/cat.yaml")) +} + func TestIsFileBeingIndexed_HTTPMatchesLocalFilename(t *testing.T) { ctx := context.Background()