Improve schema caching (#1390)

* Improve schema caching Goal is to do less redundant metaprogramming with suboptimal use of the behavior apis. * Write conformance ref errors directly to model
project-chip · Nov 13, 2024 · 6af665c · 6af665c
1 parent 7a6271f
commit 6af665c
Show file tree

Hide file tree

Showing 18 changed files with 258 additions and 84 deletions.
diff --git a/packages/model/src/aspects/Access.ts b/packages/model/src/aspects/Access.ts
@@ -83,6 +83,8 @@ export class Access extends Aspect<Access.Definition> implements Access.Ast {
         } else if (definition !== undefined && definition !== null) {
             this.set(Array.from(Access.parse(this, definition)));
         }
+
+        this.freeze();
     }
 
     /**

diff --git a/packages/model/src/aspects/Aspect.ts b/packages/model/src/aspects/Aspect.ts
@@ -11,7 +11,7 @@ import { DefinitionError } from "../common/DefinitionError.js";
  * An "aspect" is metadata about a Matter element that affects implementation behavior.  Aspects are mostly "qualities"
  * in the Matter specification except for "constraint" which is not formally described as a quality.
  */
-export abstract class Aspect<D> {
+export abstract class Aspect<D = any> {
     definition: D;
     declare errors?: DefinitionError[];
 
@@ -78,7 +78,7 @@ export abstract class Aspect<D> {
         return new constructor(definition) as This;
     }
 
-    freeze() {
+    protected freeze() {
         Object.freeze(this);
     }
 }
diff --git a/packages/model/src/aspects/Conformance.ts b/packages/model/src/aspects/Conformance.ts
@@ -55,10 +55,11 @@ export class Conformance extends Aspect<Conformance.Definition> {
             ast = definition.ast;
         }
         this.ast = ast;
+        this.freeze();
     }
 
-    validateReferences(lookup: Conformance.ReferenceResolver<boolean>) {
-        return Conformance.validateReferences(this, this.ast, lookup);
+    validateReferences(errorTarget: Conformance.ErrorTarget, lookup: Conformance.ReferenceResolver<boolean>) {
+        return Conformance.validateReferences(this, this.ast, errorTarget, lookup);
     }
 
     /**
@@ -104,7 +105,7 @@ export class Conformance extends Aspect<Conformance.Definition> {
         return Conformance.serialize(this.ast);
     }
 
-    override freeze() {
+    protected override freeze() {
         freezeAst(this.ast);
         super.freeze();
     }
@@ -262,6 +263,7 @@ export namespace Conformance {
         | "z";
 
     export type ReferenceResolver<T> = (name: string) => T;
+    export type ErrorTarget = { error(code: string, message: string): void };
 
     /**
      * Supported ways of expressing conformance (conceptually union should include Flag but that is covered by string).
@@ -280,7 +282,12 @@ export namespace Conformance {
         return serialized;
     }
 
-    export function validateReferences(conformance: Conformance, ast: Ast, resolver: ReferenceResolver<boolean>) {
+    export function validateReferences(
+        conformance: Conformance,
+        ast: Ast,
+        errorTarget: ErrorTarget,
+        resolver: ReferenceResolver<boolean>,
+    ) {
         switch (ast.type) {
             case Operator.OR:
             case Operator.XOR:
@@ -291,23 +298,23 @@ export namespace Conformance {
             case Operator.LT:
             case Operator.GTE:
             case Operator.LTE:
-                validateReferences(conformance, ast.param.lhs, resolver);
-                validateReferences(conformance, ast.param.rhs, resolver);
+                validateReferences(conformance, ast.param.lhs, errorTarget, resolver);
+                validateReferences(conformance, ast.param.rhs, errorTarget, resolver);
                 break;
 
             case Operator.NOT:
-                validateReferences(conformance, ast.param, resolver);
+                validateReferences(conformance, ast.param, errorTarget, resolver);
                 break;
 
             case Special.Group:
                 for (const a of ast.param) {
-                    validateReferences(conformance, a, resolver);
+                    validateReferences(conformance, a, errorTarget, resolver);
                 }
                 break;
 
             case Special.Name:
                 if (!resolver(ast.param)) {
-                    conformance.error(
+                    errorTarget.error(
                         `UNRESOLVED_CONFORMANCE_${ast.type.toUpperCase()}`,
                         `Conformance ${ast.type} reference "${ast.param}" does not resolve`,
                     );

diff --git a/packages/model/src/aspects/Constraint.ts b/packages/model/src/aspects/Constraint.ts
@@ -80,6 +80,8 @@ export class Constraint extends Aspect<Constraint.Definition> implements Constra
         if (ast.parts !== undefined) {
             this.parts = ast.parts.map(p => new Constraint(p));
         }
+
+        this.freeze();
     }
 
     /**
@@ -150,7 +152,7 @@ export class Constraint extends Aspect<Constraint.Definition> implements Constra
         return Constraint.serialize(this);
     }
 
-    override freeze() {
+    protected override freeze() {
         if (this.parts) {
             Object.freeze(this.parts);
         }

diff --git a/packages/model/src/aspects/Quality.ts b/packages/model/src/aspects/Quality.ts
@@ -40,6 +40,8 @@ export class Quality extends Aspect<Quality.Definition> implements Quality.Ast {
         } else {
             Object.assign(this, definition);
         }
+
+        this.freeze();
     }
 
     private parse(quality: Quality, definition: string) {

diff --git a/packages/model/src/logic/ModelTraversal.ts b/packages/model/src/logic/ModelTraversal.ts
@@ -97,7 +97,7 @@ export class ModelTraversal {
             return "struct";
         }
 
-        return this.operation(() => {
+        const getTypeNameOp = () => {
             let result: string | undefined;
             const name = model.name;
             this.visitInheritance(this.findParent(model), ancestor => {
@@ -134,7 +134,9 @@ export class ModelTraversal {
             });
 
             return result;
-        });
+        };
+
+        return this.operation(getTypeNameOp);
     }
 
     /**
@@ -166,7 +168,7 @@ export class ModelTraversal {
             return memos.bases.get(model);
         }
 
-        const base = this.operationWithDismissal(model, () => {
+        const findBaseOp = () => {
             // If I override another element (same identity and tag in parent's inheritance hierarchy) then I implicitly
             // inherit from the shadow.
             //
@@ -208,7 +210,9 @@ export class ModelTraversal {
                     return found;
                 }
             }
-        });
+        };
+
+        const base = this.operationWithDismissal(model, findBaseOp);
 
         memos?.bases.set(model, base);
 
@@ -224,12 +228,14 @@ export class ModelTraversal {
         }
         let result: Model | undefined;
 
-        this.visitInheritance(model, model => {
+        const globalBaseSearchOp = (model: Model) => {
             if (model.isGlobal) {
                 result = model;
                 return false;
             }
-        });
+        };
+
+        this.visitInheritance(model, globalBaseSearchOp);
 
         return result;
     }
@@ -296,13 +302,17 @@ export class ModelTraversal {
             return undefined;
         }
 
+        if (model.operationalShadow !== undefined) {
+            return model.operationalShadow ?? undefined;
+        }
+
         if (memos?.shadows.has(model)) {
             return memos.shadows.get(model);
         }
 
         let shadow: Model | undefined;
 
-        this.operationWithDismissal(model, () => {
+        const shadowSearchOp = () => {
             this.visitInheritance(this.findBase(this.findParent(model)), parent => {
                 if (model.id !== undefined && model.tag !== ElementTag.Command) {
                     shadow = parent.children.select(model.id, [model.tag], this.#dismissed);
@@ -315,7 +325,9 @@ export class ModelTraversal {
                     return false;
                 }
             });
-        });
+        };
+
+        this.operationWithDismissal(model, shadowSearchOp);
 
         memos?.shadows.set(model, shadow);
 
@@ -332,7 +344,7 @@ export class ModelTraversal {
             return;
         }
 
-        return this.operation(() => {
+        const findAspectOp = () => {
             let aspect = (model as any)[symbol] as Aspect<any> | undefined;
 
             const inheritedAspect = this.findAspect(this.findBase(model), symbol);
@@ -345,7 +357,8 @@ export class ModelTraversal {
             }
 
             return aspect;
-        });
+        };
+        return this.operation(findAspectOp);
     }
 
     /**
@@ -460,7 +473,7 @@ export class ModelTraversal {
         const defined = {} as Record<string, number | undefined>;
 
         let level = 0;
-        this.visitInheritance(scope, model => {
+        const childSearchVisitor = (model: Model) => {
             level++;
             for (const child of model.children) {
                 if (!tags.includes(child.tag)) {
@@ -493,7 +506,8 @@ export class ModelTraversal {
                 // Found a member
                 members.push(child);
             }
-        });
+        };
+        this.visitInheritance(scope, childSearchVisitor);
 
         return members;
     }
@@ -593,7 +607,7 @@ export class ModelTraversal {
             return memosForScope[memoKey];
         }
 
-        const type = this.operation(() => {
+        const findTypeOp = () => {
             const queue = Array<Model>(scope as Model);
             for (scope = queue.shift(); scope; scope = queue.shift()) {
                 if (scope.isTypeScope) {
@@ -615,7 +629,8 @@ export class ModelTraversal {
                     queue.push(parent);
                 }
             }
-        });
+        };
+        const type = this.operation(findTypeOp);
 
         if (memos) {
             const memosForScope = memos.types.get(scope);
@@ -845,7 +860,7 @@ export class ModelTraversal {
 
         // Next, search operational base hierarchy
         if (model.operationalBase) {
-            return this.operationWithDismissal(model, () => this.findScope(model.operationalBase));
+            return this.operationWithDismissal(model, () => this.findScope(model.operationalBase ?? undefined));
         }
 
         // Finally, fall back to the canonical MatterModel

diff --git a/packages/model/src/logic/definition-validation/ValueValidator.ts b/packages/model/src/logic/definition-validation/ValueValidator.ts
@@ -4,7 +4,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { Access, Conformance, Constraint, Quality } from "../../aspects/index.js";
+import { Access, Aspect, Conformance, Constraint, Quality } from "../../aspects/index.js";
 import { DefinitionError, FieldValue, Metatype } from "../../common/index.js";
 import { ClusterModel, Globals, ValueModel } from "../../models/index.js";
 import { ModelValidator } from "./ModelValidator.js";
@@ -23,7 +23,8 @@ export class ValueValidator<T extends ValueModel> extends ModelValidator<T> {
         this.validateProperty({ name: "quality", type: Quality });
         this.validateProperty({ name: "metatype", type: Metatype });
 
-        this.model.conformance.validateReferences(name => {
+        this.validateAspect("conformance");
+        this.model.conformance.validateReferences(this, name => {
             // Features are all caps, other names are field references
             if (name.match(/^[A-Z0-9_$]+$/)) {
                 // Feature lookup
@@ -35,7 +36,6 @@ export class ValueValidator<T extends ValueModel> extends ModelValidator<T> {
             }
         });
 
-        this.validateAspect("conformance");
         this.validateAspect("constraint");
         this.validateAspect("access");
         this.validateAspect("quality");
@@ -47,7 +47,7 @@ export class ValueValidator<T extends ValueModel> extends ModelValidator<T> {
     }
 
     private validateAspect(name: string) {
-        const aspect = (this.model as any)[name];
+        const aspect = (this.model as any)[name] as Aspect;
         if (aspect?.errors) {
             aspect.errors.forEach((e: DefinitionError) => this.model.error(e.code, e.message));
         }

diff --git a/packages/model/src/models/Aspects.ts b/packages/model/src/models/Aspects.ts
@@ -4,19 +4,40 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import type { Aspect } from "#aspects/Aspect.js";
 import { ModelTraversal } from "../logic/ModelTraversal.js";
 import { Model } from "./Model.js";
 
+// Aspects are immutable, there are not many permutations, and their definitions are largely normalized strings.  So we
+// cache them to keep the object count down
+const aspectCache: Record<string, Record<string, Aspect>> = {
+    Access: {},
+    Conformance: {},
+    Constraint: {},
+    Quality: {},
+};
+
+const emptyAspects: Record<string, Aspect> = {};
+
 export namespace Aspects {
-    export function getAspect<T>(model: Model, symbol: symbol, constructor: new (definition: any) => T) {
-        return ((model as any)[symbol] as T) || ((model as any)[symbol] = new constructor(undefined));
+    export function getAspect<T extends Aspect>(model: Model, symbol: symbol, constructor: new (definition: any) => T) {
+        const aspect = (model as any)[symbol] ?? emptyAspects[constructor.name];
+        if (aspect) {
+            return aspect as T;
+        }
+        return (emptyAspects[constructor.name] = new constructor(undefined));
     }
 
     export function setAspect(model: Model, symbol: symbol, constructor: new (definition: any) => any, value: any) {
         if (value instanceof constructor) {
             (model as any)[symbol] = value;
         } else {
-            (model as any)[symbol] = new constructor(value);
+            const cacheKey = typeof value === "string" ? value : JSON.stringify(value);
+            let aspect = aspectCache[constructor.name][value];
+            if (!aspect) {
+                aspect = aspectCache[constructor.name][cacheKey] = new constructor(value);
+            }
+            (model as any)[symbol] = aspect;
         }
     }
 
@@ -29,7 +50,11 @@ export namespace Aspects {
         }
     }
 
-    export function getEffectiveAspect<T>(model: Model, symbol: symbol, constructor: new (definition: any) => T) {
+    export function getEffectiveAspect<T extends Aspect>(
+        model: Model,
+        symbol: symbol,
+        constructor: new (definition: any) => T,
+    ) {
         const aspect = new ModelTraversal().findAspect(model, symbol) as T | undefined;
         if (aspect) {
             return aspect;

diff --git a/packages/model/src/models/ClusterModel.ts b/packages/model/src/models/ClusterModel.ts
@@ -202,11 +202,6 @@ export class ClusterModel extends Model<ClusterElement> implements ClusterElemen
         return result as ClusterElement;
     }
 
-    override freeze() {
-        this.quality.freeze();
-        super.freeze();
-    }
-
     constructor(definition: ClusterElement.Properties) {
         super(definition);
-Original file line number
+Diff line change
@@ Expand Up @@
             } else if (definition !== undefined && definition !== null) {
                 this.set(Array.from(Access.parse(this, definition)));
             }
+            this.freeze();
         }
         /**
@@ Expand Down @@