From 90b107ea6ac48dc3184a228394a15696775a9d4c Mon Sep 17 00:00:00 2001 From: Joseph Axisa Date: Sat, 24 Oct 2026 17:29:07 -0700 Subject: [PATCH] perf: reuse a reference's quick hash before building its schema With UseSchemaQuickHash, every non-circular reference built the schema it points to and only then found its quick hash in SchemaQuickHashMap. Many references point at the same schema, so most of those builds were thrown away: comparing test_specs/stripe.yaml with a copy carrying 300 edits, 3,954 of 5,602 reference builds during CompareDocuments found the hash already cached. Work out the key the built schema would use from the proxy and look it up first, building only on a miss. The key comes from the same function the schema uses. A node that is itself a $ref is re-attributed while building, so it still goes through Schema(). With quick hash on, CompareDocuments takes about 12% less time and 13% less memory on the stripe.yaml comparison, and about half on docusignv3.1.json. Quick hash off is unchanged. --- datamodel/low/base/schema_hash.go | 39 +++++++------ datamodel/low/base/schema_proxy.go | 30 +++++++++- datamodel/low/base/schema_proxy_test.go | 77 +++++++++++++++++++++++++ what-changed/what_changed_test.go | 66 +++++++++++++++++++++ 4 files changed, 194 insertions(+), 18 deletions(-) diff --git a/datamodel/low/base/schema_hash.go b/datamodel/low/base/schema_hash.go index 04205304..887a4a33 100644 --- a/datamodel/low/base/schema_hash.go +++ b/datamodel/low/base/schema_hash.go @@ -10,7 +10,9 @@ import ( "strings" "sync" + "github.com/pb33f/go-yaml" "github.com/pb33f/libopenapi/datamodel/low" + "github.com/pb33f/libopenapi/index" "github.com/pb33f/libopenapi/orderedmap" ) @@ -412,26 +414,29 @@ func hashSchemaDependentRequired(h *maphash.Hash, label string, m *orderedmap.Ma } func (s *Schema) quickHashKey() string { - // The key identifies a node by file plus position, so the path must come from the file - // RootNode is in. GetIndex() may have been re-attributed to the file a $ref resolves to, - // which would pair a resolved-file path with a referring-file line and column: two - // different $refs at the same position in different files pointing into one shared file - // would then collide and return each other's hash. - // refIndex is assigned unconditionally in Build alongside Index, so it is nil only when - // the schema was never built, in which case Index is nil too and there is no path either way. - idx := s.refIndex + return quickHashKeyOf(s.refIndex, s.Index, s.RootNode) +} + +// quickHashKeyOf builds the quick-hash key for a schema whose root node is root, read from the file of +// refIndex, in the document of idx. The key identifies a node by file plus position, so the path must come +// from the file root is in. Index may have been re-attributed to the file a $ref resolves to, which would +// pair a resolved-file path with a referring-file line and column: two different $refs at the same position +// in different files pointing into one shared file would then collide and return each other's hash. +// refIndex is assigned unconditionally in Build alongside Index, so it is nil only when the schema was never +// built, in which case Index is nil too and there is no path either way. +func quickHashKeyOf(refIndex, idx *index.SpecIndex, root *yaml.Node) string { path := "" - if idx != nil { - path = idx.GetSpecAbsolutePath() + if refIndex != nil { + path = refIndex.GetSpecAbsolutePath() } cfID := "root" - if s.Index != nil { - if s.Index.GetRolodex() != nil { - if s.Index.GetRolodex().GetId() != "" { - cfID = s.Index.GetRolodex().GetId() + if idx != nil { + if idx.GetRolodex() != nil { + if idx.GetRolodex().GetId() != "" { + cfID = idx.GetRolodex().GetId() } } else { - cfID = s.Index.GetConfig().GetId() + cfID = idx.GetConfig().GetId() } } @@ -439,9 +444,9 @@ func (s *Schema) quickHashKey() string { keyBuf.Grow(len(path) + len(cfID) + 16) keyBuf.WriteString(path) keyBuf.WriteByte(':') - keyBuf.WriteString(strconv.Itoa(s.RootNode.Line)) + keyBuf.WriteString(strconv.Itoa(root.Line)) keyBuf.WriteByte(':') - keyBuf.WriteString(strconv.Itoa(s.RootNode.Column)) + keyBuf.WriteString(strconv.Itoa(root.Column)) keyBuf.WriteByte(':') keyBuf.WriteString(cfID) return keyBuf.String() diff --git a/datamodel/low/base/schema_proxy.go b/datamodel/low/base/schema_proxy.go index c2eae19f..66290012 100644 --- a/datamodel/low/base/schema_proxy.go +++ b/datamodel/low/base/schema_proxy.go @@ -267,7 +267,8 @@ func (sp *SchemaProxy) Schema() *Schema { // there were no errors during build, then nil will be returned. // // Thread safety: GetBuildError() is safe to call concurrently only after Schema() has been called at least once -// on this proxy (from any goroutine). All standard code paths (Hash(), high-level Schema()) call Schema() first. +// on this proxy (from any goroutine). The high-level Schema() calls Schema() first; Hash() may not, when a quick +// hash for the referenced schema is already cached. func (sp *SchemaProxy) GetBuildError() error { return sp.buildError } @@ -363,6 +364,10 @@ func (sp *SchemaProxy) computeHash() uint64 { cfg := sp.getSpecConfig() if cfg != nil && cfg.UseSchemaQuickHash { if !CheckSchemaProxyForCircularRefs(sp) { + // many references point at the same schema; reuse its quick hash before building another copy. + if hash, ok := sp.cachedQuickHash(); ok { + return hash + } sch = sp.Schema() if sch != nil { return sch.QuickHash() @@ -475,3 +480,26 @@ func (sp *SchemaProxy) attemptPropertyMerging(node *yaml.Node, config *datamodel return merged } + +// cachedQuickHash returns the quick hash already stored for the schema this reference would build, without +// building it. It only answers when the key the built schema would use can be worked out from the proxy: a node +// that is itself a $ref is re-attributed to another index while building (and is the only kind of node +// referenced-property merging changes), so that is left to Schema(). +func (sp *SchemaProxy) cachedQuickHash() (uint64, bool) { + if sp.vn == nil { + return 0, false + } + root := utils.ResolveMergeNodes(sp.vn) + if root == nil { + return 0, false + } + if isRef, _, _ := utils.IsNodeRefValue(root); isRef { + return 0, false + } + if v, ok := SchemaQuickHashMap.Load(quickHashKeyOf(sp.idx, sp.idx, root)); ok { + if hash, isHash := v.(uint64); isHash { + return hash, true + } + } + return 0, false +} diff --git a/datamodel/low/base/schema_proxy_test.go b/datamodel/low/base/schema_proxy_test.go index 577ffbad..99958f54 100644 --- a/datamodel/low/base/schema_proxy_test.go +++ b/datamodel/low/base/schema_proxy_test.go @@ -921,3 +921,80 @@ properties: recomputedHash := sch.Hash() assert.NotEqual(t, uint64(0), recomputedHash) } + +// quickHashRefProxy is a reference proxy as the model builds it: its value node is the schema it points to. +func quickHashRefProxy(t *testing.T, target string, cfg *index.SpecIndexConfig) *SchemaProxy { + t.Helper() + var node yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(target), &node)) + cfg.UseSchemaQuickHash = true + idx := index.NewSpecIndexWithConfig(&node, cfg) + rolodex := index.NewRolodex(cfg) + idx.SetRolodex(rolodex) + rolodex.SetRootIndex(idx) + + sp := new(SchemaProxy) + sp.vn = node.Content[0] + sp.idx = idx + sp.ctx = context.Background() + r := low.Reference{} + r.SetReference("#/components/schemas/Pet", &yaml.Node{}) + sp.Reference = r + return sp +} + +func TestSchemaProxy_QuickHash_ReusesCachedHashWithoutBuilding(t *testing.T) { + ClearSchemaQuickHashMap() + defer ClearSchemaQuickHashMap() + sp := quickHashRefProxy(t, "type: object\nproperties:\n name:\n type: string\n", &index.SpecIndexConfig{}) + + SchemaQuickHashMap.Store(quickHashKeyOf(sp.idx, sp.idx, sp.vn), uint64(42)) + + assert.Equal(t, uint64(42), sp.Hash()) + assert.Nil(t, sp.rendered.Load(), "the referenced schema should not be built") +} + +func TestSchemaProxy_QuickHash_BuildsOnMiss(t *testing.T) { + ClearSchemaQuickHashMap() + defer ClearSchemaQuickHashMap() + sp := quickHashRefProxy(t, "type: object\nproperties:\n name:\n type: string\n", &index.SpecIndexConfig{}) + + hash := sp.Hash() + + built := sp.rendered.Load() + require.NotNil(t, built) + assert.Equal(t, built.QuickHash(), hash) + // the key worked out from the proxy is the one the built schema uses. + assert.Equal(t, built.quickHashKey(), quickHashKeyOf(sp.idx, sp.idx, sp.vn)) +} + +func TestSchemaProxy_CachedQuickHash_Declines(t *testing.T) { + ClearSchemaQuickHashMap() + defer ClearSchemaQuickHashMap() + target := "type: object\nproperties:\n name:\n type: string\n" + + // a node that is itself a $ref is re-attributed while building, with or without property merging. + for _, cfg := range []*index.SpecIndexConfig{{}, {MergeReferencedProperties: true}} { + sp := quickHashRefProxy(t, "$ref: '#/components/schemas/Other'\ndescription: sibling\n", cfg) + SchemaQuickHashMap.Store(quickHashKeyOf(sp.idx, sp.idx, sp.vn), uint64(42)) + _, ok := sp.cachedQuickHash() + assert.False(t, ok) + } + + // nothing to build. + sp := new(SchemaProxy) + _, ok := sp.cachedQuickHash() + assert.False(t, ok) + + // an alias with nothing behind it. + sp = quickHashRefProxy(t, target, &index.SpecIndexConfig{}) + sp.vn = &yaml.Node{Kind: yaml.AliasNode} + _, ok = sp.cachedQuickHash() + assert.False(t, ok) + + // a value that isn't a hash. + sp = quickHashRefProxy(t, target, &index.SpecIndexConfig{}) + SchemaQuickHashMap.Store(quickHashKeyOf(sp.idx, sp.idx, sp.vn), "not a hash") + _, ok = sp.cachedQuickHash() + assert.False(t, ok) +} diff --git a/what-changed/what_changed_test.go b/what-changed/what_changed_test.go index 5d76f948..2654c678 100644 --- a/what-changed/what_changed_test.go +++ b/what-changed/what_changed_test.go @@ -12,6 +12,7 @@ import ( "github.com/pb33f/libopenapi/datamodel" v2 "github.com/pb33f/libopenapi/datamodel/low/v2" v3 "github.com/pb33f/libopenapi/datamodel/low/v3" + "github.com/pb33f/libopenapi/what-changed/model" "github.com/pb33f/testify/assert" "github.com/pb33f/testify/require" ) @@ -388,3 +389,68 @@ func TestCompareOpenAPIDocuments_CrossFileCircularReference(t *testing.T) { assert.Equal(t, 1, changes.TotalChanges()) assert.Equal(t, 1, changes.TotalBreakingChanges()) } + +// Many references to one schema in a document with a reference loop: with quick hash, references after the +// first reuse the cached hash of the schema they point to rather than building it again. The result must be the +// same as without quick hash. +func TestCompareOpenAPIDocuments_QuickHashCyclicSharedRefs(t *testing.T) { + spec := func(nameType string) []byte { + return []byte(`openapi: 3.1.0 +info: + title: shared refs + version: '1' +paths: {} +components: + schemas: + Pet: + type: object + properties: + name: + type: ` + nameType + ` + Owner: + type: object + properties: + pet: + $ref: '#/components/schemas/Pet' + pets: + type: array + items: + $ref: '#/components/schemas/Pet' + Shop: + type: object + properties: + featured: + $ref: '#/components/schemas/Pet' + owner: + $ref: '#/components/schemas/Owner' + Node: + type: object + properties: + next: + $ref: '#/components/schemas/Link' + Link: + type: object + properties: + node: + $ref: '#/components/schemas/Node'`) + } + compare := func(quick bool) *model.DocumentChanges { + config := datamodel.NewDocumentConfiguration() + config.UseSchemaQuickHash = quick + load := func(b []byte) *v3.Document { + info, err := datamodel.ExtractSpecInfo(b) + require.NoError(t, err) + doc, err := v3.CreateDocumentFromConfig(info, config) + require.NoError(t, err) + return doc + } + return CompareOpenAPIDocuments(load(spec("string")), load(spec("integer"))) + } + + withQuickHash, without := compare(true), compare(false) + require.NotNil(t, withQuickHash) + require.NotNil(t, without) + assert.Equal(t, without.TotalChanges(), withQuickHash.TotalChanges()) + assert.Equal(t, without.TotalBreakingChanges(), withQuickHash.TotalBreakingChanges()) + assert.Equal(t, 1, withQuickHash.TotalBreakingChanges()) // Pet.name: string -> integer +}