openapi3: store field locations in a slice, not a map

.github/docs/openapi3.txt

@@ -991,6 +991,38 @@ func (e *ExtraSiblingFieldsError) Code() string
 
 func (e *ExtraSiblingFieldsError) Error() string
 
+type FieldLocations []Location
+    FieldLocations holds the locations of a collection's scalar fields, in the
+    order they appear in the document.
+
+    It is a slice rather than a map[string]Location because a collection carries
+    only a handful of fields, while a Go map allocates a whole bucket per
+    collection whatever it holds. On a large document that overhead dominated
+    the retained size of a parsed spec. Each Location already carries its Name,
+    so the lookup key costs nothing extra here.
+
+func (f FieldLocations) Get(name string) Location
+    Get returns the location of the named field, or the zero Location when the
+    field has none. Use Lookup to tell an absent field from a zero location.
+
+func (f FieldLocations) Lookup(name string) (Location, bool)
+    Lookup returns the location of the named field and whether it was found.
+    The scan is linear: collections have few fields, and a linear scan over a
+    contiguous slice beats a map lookup at these sizes.
+
+    Deliberately a hand-written loop rather than slices.IndexFunc: the closure
+    does not inline, so IndexFunc pays a call per element. Measured on 3/6/12
+    fields it is 5-100% slower on a hit and 2-3x slower on a miss, and misses
+    are the common case here (most fields carry no recorded location).
+
+func (f FieldLocations) MarshalJSON() ([]byte, error)
+    MarshalJSON keeps the serialized shape a name-keyed object, as it was when
+    this was a map, so the change is invisible to anything reading the output.
+
+func (f *FieldLocations) UnmarshalJSON(data []byte) error
+    UnmarshalJSON reads the name-keyed object written by MarshalJSON. Entries
+    are sorted by name, since a JSON object carries no order to restore.
+
 type FieldVersionMismatchError struct {
 	// Field is the field name flagged (e.g. "summary", "identifier",
 	// "$defs", "prefixItems", "contains", ...).
@@ -1872,14 +1904,23 @@ func (e *OperationValidationError) Unwrap() error
 
 type Origin struct {
 	Key       *Location             `json:"key,omitempty" yaml:"key,omitempty"`
-	Fields    map[string]Location   `json:"fields,omitempty" yaml:"fields,omitempty"`
+	Fields    FieldLocations        `json:"fields,omitempty" yaml:"fields,omitempty"`
 	Sequences map[string][]Location `json:"sequences,omitempty" yaml:"sequences,omitempty"`
 }
-    Origin contains the origin of a collection. Key is the location of the
-    collection itself. Fields is a map of the location of each scalar field
+    Origin contains the origin of a collection. Key is the location of
+    the collection itself. Fields holds the location of each scalar field
     in the collection. Sequences is a map of the location of each item in
     sequence-valued fields.
 
+    Sequences stays a map although Fields is a slice, which is deliberate.
+    FieldLocations drops the map because Location.Name already carries the key,
+    so the map was storing information the value repeated. Here Location.Name
+    holds the *item's* value (an enum member, a required property) while the key
+    is the *field's* name ("enum", "required", "tags"), so a slice would need a
+    wrapper type invented to hold it. The memory argument is also much weaker:
+    only a collection with a sequence-valued field allocates one at all,
+    which measured at 5% of collections on a large spec, and a nil map is free.
+
 type Parameter struct {
 	Extensions map[string]any `json:"-" yaml:"-"`
 	Origin     *Origin        `json:"-" yaml:"-"`

openapi3/loader.go

@@ -149,8 +149,26 @@ func (loader *Loader) loadSingleElementFromURI(ref string, rootPath *url.URL, el
 
 // rememberOriginTree retains doc's origin tree for attachOriginToResolved.
 // tree is nil when IncludeOrigin is off or the data took the json path.
+//
+// The tree is kept only for a document a $ref can reach into untyped, which is
+// what attachOriginToResolved exists to re-origin. In practice that means a
+// file of shared fragments, whose top level is the fragment name itself rather
+// than the fields of an OpenAPI Object:
+//
+//	User:            # a $ref to "./schemas.yaml#/User" lands here, untyped
+//	  type: object
+//
+// Anything OpenAPI defines a field for resolves through typed structures and
+// keeps its origins on the way, so its tree could never be read. That includes
+// a referenced document that is itself an OpenAPI Object: a $ref to
+// "#/components/schemas/User" needs no tree. (A top-level x- extension is
+// undefined by the same rule, so a document carrying one keeps its tree too,
+// whether or not anything ever points at it.)
+//
+// Worth the condition: on a 22 MB spec the retained tree was a third of
+// everything the loader held.
 func (loader *Loader) rememberOriginTree(doc *T, tree *originTree) {
-	if tree == nil {
+	if tree == nil || len(doc.Extensions) == 0 {
 		return
 	}
 	if loader.originTrees == nil {

openapi3/origin.go

@@ -1,8 +1,9 @@
 package openapi3
 
 import (
+	"encoding/json"
 	"reflect"
-	"sort"
+	"slices"
 	"strings"
 
 	"github.com/oasdiff/yaml"
@@ -12,14 +13,86 @@ var originPtrType = reflect.TypeFor[*Origin]()
 
 // Origin contains the origin of a collection.
 // Key is the location of the collection itself.
-// Fields is a map of the location of each scalar field in the collection.
+// Fields holds the location of each scalar field in the collection.
 // Sequences is a map of the location of each item in sequence-valued fields.
+//
+// Sequences stays a map although Fields is a slice, which is deliberate.
+// FieldLocations drops the map because Location.Name already carries the key,
+// so the map was storing information the value repeated. Here Location.Name
+// holds the *item's* value (an enum member, a required property) while the key
+// is the *field's* name ("enum", "required", "tags"), so a slice would need a
+// wrapper type invented to hold it. The memory argument is also much weaker:
+// only a collection with a sequence-valued field allocates one at all, which
+// measured at 5% of collections on a large spec, and a nil map is free.
 type Origin struct {
 	Key       *Location             `json:"key,omitempty" yaml:"key,omitempty"`
-	Fields    map[string]Location   `json:"fields,omitempty" yaml:"fields,omitempty"`
+	Fields    FieldLocations        `json:"fields,omitempty" yaml:"fields,omitempty"`
 	Sequences map[string][]Location `json:"sequences,omitempty" yaml:"sequences,omitempty"`
 }
 
+// FieldLocations holds the locations of a collection's scalar fields, in the
+// order they appear in the document.
+//
+// It is a slice rather than a map[string]Location because a collection carries
+// only a handful of fields, while a Go map allocates a whole bucket per
+// collection whatever it holds. On a large document that overhead dominated
+// the retained size of a parsed spec. Each Location already carries its Name,
+// so the lookup key costs nothing extra here.
+type FieldLocations []Location
+
+// Get returns the location of the named field, or the zero Location when the
+// field has none. Use Lookup to tell an absent field from a zero location.
+func (f FieldLocations) Get(name string) Location {
+	loc, _ := f.Lookup(name)
+	return loc
+}
+
+// Lookup returns the location of the named field and whether it was found.
+// The scan is linear: collections have few fields, and a linear scan over a
+// contiguous slice beats a map lookup at these sizes.
+//
+// Deliberately a hand-written loop rather than slices.IndexFunc: the closure
+// does not inline, so IndexFunc pays a call per element. Measured on 3/6/12
+// fields it is 5-100% slower on a hit and 2-3x slower on a miss, and misses
+// are the common case here (most fields carry no recorded location).
+func (f FieldLocations) Lookup(name string) (Location, bool) {
+	for i := range f {
+		if f[i].Name == name {
+			return f[i], true
+		}
+	}
+	return Location{}, false
+}
+
+// MarshalJSON keeps the serialized shape a name-keyed object, as it was when
+// this was a map, so the change is invisible to anything reading the output.
+func (f FieldLocations) MarshalJSON() ([]byte, error) {
+	m := make(map[string]Location, len(f))
+	for _, loc := range f {
+		m[loc.Name] = loc
+	}
+	return json.Marshal(m)
+}
+
+// UnmarshalJSON reads the name-keyed object written by MarshalJSON. Entries are
+// sorted by name, since a JSON object carries no order to restore.
+func (f *FieldLocations) UnmarshalJSON(data []byte) error {
+	var m map[string]Location
+	if err := json.Unmarshal(data, &m); err != nil {
+		return err
+	}
+	out := make(FieldLocations, 0, len(m))
+	for name, loc := range m {
+		if loc.Name == "" {
+			loc.Name = name
+		}
+		out = append(out, loc)
+	}
+	slices.SortFunc(out, func(a, b Location) int { return strings.Compare(a.Name, b.Name) })
+	*f = out
+	return nil
+}
+
 // Location is a struct that contains the location of a field.
 type Location struct {
 	File   string `json:"file,omitempty" yaml:"file,omitempty"`
@@ -61,17 +134,17 @@ func originFromSeq(s []any) *Origin {
 	nf := toInt(s[idx])
 	idx++
 	if nf > 0 && idx+nf*3 <= len(s) {
-		o.Fields = make(map[string]Location, nf)
+		o.Fields = make(FieldLocations, 0, nf)
 		for range nf {
 			fname, _ := s[idx].(string)
 			delta := toInt(s[idx+1])
 			col := toInt(s[idx+2])
-			o.Fields[fname] = Location{
+			o.Fields = append(o.Fields, Location{
 				File:   file,
 				Line:   keyLine + delta,
 				Column: col,
 				Name:   fname,
-			}
+			})
 			idx += 3
 		}
 	}
@@ -148,10 +221,13 @@ func isScalarValuedMapField(v reflect.Value) bool {
 	return false
 }
 
-// recordMapKeyLocations copies the map-key locations from a scalar-valued map's
+// recordMapKeyLocations moves the map-key locations from a scalar-valued map's
 // own subtree onto parentOrigin.Sequences[field], so each key is addressable by
 // name (the same shape used for sequence items). It is a no-op when the child
 // carries no origin data. Keys are sorted for deterministic output.
+//
+// childOrigin is discarded here, and nothing else holds its Fields, so the
+// slice is sorted and handed over in place rather than copied.
 func recordMapKeyLocations(parentOrigin *Origin, field string, childTree *yaml.OriginTree) {
 	s, ok := childTree.Origin.([]any)
 	if !ok {
@@ -161,11 +237,8 @@ func recordMapKeyLocations(parentOrigin *Origin, field string, childTree *yaml.O
 	if childOrigin == nil || len(childOrigin.Fields) == 0 {
 		return
 	}
-	locs := make([]Location, 0, len(childOrigin.Fields))
-	for _, loc := range childOrigin.Fields {
-		locs = append(locs, loc)
-	}
-	sort.Slice(locs, func(i, j int) bool { return locs[i].Name < locs[j].Name })
+	locs := childOrigin.Fields
+	slices.SortFunc(locs, func(a, b Location) int { return strings.Compare(a.Name, b.Name) })
 	if parentOrigin.Sequences == nil {
 		parentOrigin.Sequences = make(map[string][]Location)
 	}

openapi3/origin_external_ref_test.go

@@ -35,14 +35,14 @@ func TestOrigin_ExternalRefToArbitraryTopLevelKey(t *testing.T) {
 		Name:    "User",
 		EndLine: 7, EndColumn: 19,
 	}, *user.Origin.Key, "the key origin spans the whole User block in arbitrary_key_schemas.yaml")
-	require.Equal(t, 2, user.Origin.Fields["type"].Line, "field origins are attached too")
+	require.Equal(t, 2, user.Origin.Fields.Get("type").Line, "field origins are attached too")
 
 	// the subtree gets origins as well, with the same file
 	id := user.Properties["id"].Value
 	require.NotNil(t, id.Origin)
 	require.Equal(t, user.Origin.Key.File, id.Origin.Key.File)
 	require.Equal(t, 4, id.Origin.Key.Line, "the id property's own line")
-	require.Equal(t, 5, id.Origin.Fields["type"].Line)
+	require.Equal(t, 5, id.Origin.Fields.Get("type").Line)
 }
 
 // Re-attaching origins reuses the origin tree retained at load time: resolving
@@ -101,6 +101,6 @@ User:
 	require.Equal(t, "User", user.Origin.Key.Name)
 	require.Equal(t, 14, user.Origin.Key.Line, "the User: line in the document above")
 	require.Equal(t, 17, user.Origin.Key.EndLine, "the block's last line")
-	require.Equal(t, 15, user.Origin.Fields["type"].Line)
+	require.Equal(t, 15, user.Origin.Fields.Get("type").Line)
 	require.Empty(t, user.Origin.Key.File, "a document loaded from data has no file")
 }

openapi3/origin_retention_test.go

@@ -0,0 +1,71 @@
+package openapi3
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// An ordinary spec never consults its origin tree, so the loader does not keep
+// one. Origins are still attached to the document itself; what goes away is the
+// second copy the loader used to hold for the lifetime of the loader, which on
+// a large spec was a third of everything it retained.
+func TestOriginTree_NotRetainedForAnOrdinarySpec(t *testing.T) {
+	loader := NewLoader()
+	loader.IncludeOrigin = true
+	loader.Context = t.Context()
+
+	doc, err := loader.LoadFromFile("testdata/origin/simple.yaml")
+	require.NoError(t, err)
+
+	require.NotNil(t, doc.Origin, "origins are still attached to the document")
+	require.Empty(t, loader.originTrees, "no tree is retained: nothing could read it")
+}
+
+// The tree is kept for exactly the documents that can consult it, and this OAD
+// contains one of each: the entry document has only fields OpenAPI defines,
+// while the file it $refs is a shared fragment whose top level is the schema
+// name "User", which is what arrives untyped and what attachOriginToResolved
+// walks the tree for.
+//
+// The retained tree still does its job here, so this pins the saving and the
+// capability together: dropping the tree for the referenced file would leave
+// the resolved schema without an origin.
+func TestOriginTree_RetainedOnlyWhereItCanBeRead(t *testing.T) {
+	loader := NewLoader()
+	loader.IsExternalRefsAllowed = true
+	loader.IncludeOrigin = true
+	loader.Context = t.Context()
+
+	doc, err := loader.LoadFromFile("testdata/origin/arbitrary_key.yaml")
+	require.NoError(t, err)
+
+	require.NotContains(t, loader.originTrees, doc,
+		"the entry document has only fields OpenAPI defines, so no $ref reaches into it untyped")
+	require.Len(t, loader.originTrees, 1, "only the shared-fragment file keeps its tree")
+	for retained := range loader.originTrees {
+		require.NotEmpty(t, retained.Extensions,
+			"a tree is kept only for a document with top-level fields OpenAPI does not define")
+	}
+
+	// The capability the retained tree exists for: the resolved schema carries
+	// the origin of its own file, not of the $ref site.
+	schema := doc.Paths.Find("/users").Get.Responses.Value("200").Value.
+		Content["application/json"].Schema.Value
+	require.NotNil(t, schema.Origin, "the resolved schema keeps its origin")
+	require.Contains(t, schema.Origin.Key.File, "arbitrary_key_schemas.yaml")
+}
+
+// Without IncludeOrigin there is no tree to begin with, so the new condition
+// cannot change anything here.
+func TestOriginTree_NotRetainedWhenOriginsAreOff(t *testing.T) {
+	loader := NewLoader()
+	loader.IsExternalRefsAllowed = true
+	loader.Context = t.Context()
+
+	doc, err := loader.LoadFromFile("testdata/origin/arbitrary_key.yaml")
+	require.NoError(t, err)
+
+	require.Nil(t, doc.Origin)
+	require.Empty(t, loader.originTrees)
+}