From f29ce906326456d09776aacafe2f664a67d6f8cc Mon Sep 17 00:00:00 2001 From: Andrey Sobolev Date: Wed, 8 Oct 2025 16:02:14 +0700 Subject: [PATCH] Improve hierarchy + tests Add tests for hierarchy and few performance/memory optimizations. --- .../core/main_2025-10-08-09-02.json | 10 + packages/core/src/__tests__/hierarchy.test.ts | 222 ++++++++++++++++++ packages/core/src/hierarchy.ts | 44 ++-- 3 files changed, 252 insertions(+), 24 deletions(-) create mode 100644 common/changes/@hcengineering/core/main_2025-10-08-09-02.json diff --git a/common/changes/@hcengineering/core/main_2025-10-08-09-02.json b/common/changes/@hcengineering/core/main_2025-10-08-09-02.json new file mode 100644 index 0000000000..87ed707a04 --- /dev/null +++ b/common/changes/@hcengineering/core/main_2025-10-08-09-02.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@hcengineering/core", + "comment": "Hierarchy improvements", + "type": "patch" + } + ], + "packageName": "@hcengineering/core" +} \ No newline at end of file diff --git a/packages/core/src/__tests__/hierarchy.test.ts b/packages/core/src/__tests__/hierarchy.test.ts index babc79a352..ac18197a8a 100644 --- a/packages/core/src/__tests__/hierarchy.test.ts +++ b/packages/core/src/__tests__/hierarchy.test.ts @@ -106,4 +106,226 @@ describe('hierarchy', () => { spyMixinClass.mockReset() spyMixinClass.mockRestore() }) + + // Memory optimization tests - ancestors stored as array + it('getAncestors should return array directly', async () => { + const hierarchy = prepare() + const ancestors = hierarchy.getAncestors(core.class.TxCreateDoc) + + // Verify it's an array + expect(Array.isArray(ancestors)).toBeTruthy() + + // Verify it contains expected ancestors + expect(ancestors).toContain(core.class.TxCreateDoc) + expect(ancestors).toContain(core.class.TxCUD) + expect(ancestors).toContain(core.class.Tx) + expect(ancestors).toContain(core.class.Doc) + expect(ancestors).toContain(core.class.Obj) + + // Verify order is consistent + const indexTx = ancestors.indexOf(core.class.Tx) + const indexDoc = ancestors.indexOf(core.class.Doc) + expect(indexDoc).toBeGreaterThan(indexTx) + }) + + it('isDerived should work with array-based ancestors', async () => { + const hierarchy = prepare() + + // Test various inheritance chains + expect(hierarchy.isDerived(core.class.TxCreateDoc, core.class.Tx)).toBeTruthy() + expect(hierarchy.isDerived(core.class.TxCreateDoc, core.class.TxCUD)).toBeTruthy() + expect(hierarchy.isDerived(core.class.TxCreateDoc, core.class.Doc)).toBeTruthy() + expect(hierarchy.isDerived(core.class.TxCreateDoc, core.class.Obj)).toBeTruthy() + + // Test self-derivation (class is in its own ancestors) + expect(hierarchy.isDerived(core.class.TxCreateDoc, core.class.TxCreateDoc)).toBeTruthy() + + // Test non-derived classes + expect(hierarchy.isDerived(core.class.TxCreateDoc, core.class.Space)).toBeFalsy() + expect(hierarchy.isDerived(core.class.Space, core.class.Tx)).toBeFalsy() + + // Test with mixins + expect(hierarchy.isDerived(test.class.TestComment, core.class.AttachedDoc)).toBeTruthy() + expect(hierarchy.isDerived(test.mixin.TaskMixinTodos, test.class.Task)).toBeTruthy() + }) + + it('should handle deep inheritance chains efficiently', async () => { + const hierarchy = prepare() + + // TxCreateDoc has a chain: TxCreateDoc -> TxCUD -> Tx -> Doc -> Obj + const ancestors = hierarchy.getAncestors(core.class.TxCreateDoc) + expect(ancestors.length).toBeGreaterThanOrEqual(5) + + // All intermediate classes should be present + expect(hierarchy.isDerived(core.class.TxCreateDoc, core.class.TxCUD)).toBeTruthy() + expect(hierarchy.isDerived(core.class.TxCUD, core.class.Tx)).toBeTruthy() + expect(hierarchy.isDerived(core.class.Tx, core.class.Doc)).toBeTruthy() + expect(hierarchy.isDerived(core.class.Doc, core.class.Obj)).toBeTruthy() + }) + + // Classifier properties tests - Map-based storage + it('getClassifierProp and setClassifierProp should work with Map', async () => { + const hierarchy = prepare() + + // Set a property + hierarchy.setClassifierProp(core.class.Space, 'testProp', 'testValue') + + // Get the property + const value = hierarchy.getClassifierProp(core.class.Space, 'testProp') + expect(value).toBe('testValue') + + // Update the property + hierarchy.setClassifierProp(core.class.Space, 'testProp', 'updatedValue') + const updatedValue = hierarchy.getClassifierProp(core.class.Space, 'testProp') + expect(updatedValue).toBe('updatedValue') + }) + + it('should handle multiple properties per classifier', async () => { + const hierarchy = prepare() + + // Set multiple properties + hierarchy.setClassifierProp(core.class.Space, 'prop1', 'value1') + hierarchy.setClassifierProp(core.class.Space, 'prop2', 'value2') + hierarchy.setClassifierProp(core.class.Space, 'prop3', 42) + hierarchy.setClassifierProp(core.class.Space, 'prop4', { nested: 'object' }) + + // Verify all properties are stored correctly + expect(hierarchy.getClassifierProp(core.class.Space, 'prop1')).toBe('value1') + expect(hierarchy.getClassifierProp(core.class.Space, 'prop2')).toBe('value2') + expect(hierarchy.getClassifierProp(core.class.Space, 'prop3')).toBe(42) + expect(hierarchy.getClassifierProp(core.class.Space, 'prop4')).toEqual({ nested: 'object' }) + + // Verify undefined for non-existent property + expect(hierarchy.getClassifierProp(core.class.Space, 'nonExistent')).toBeUndefined() + }) + + it('should isolate properties between different classifiers', async () => { + const hierarchy = prepare() + + // Set properties on different classifiers + hierarchy.setClassifierProp(core.class.Space, 'name', 'Space') + hierarchy.setClassifierProp(core.class.Doc, 'name', 'Doc') + hierarchy.setClassifierProp(test.class.Task, 'name', 'Task') + + // Verify isolation + expect(hierarchy.getClassifierProp(core.class.Space, 'name')).toBe('Space') + expect(hierarchy.getClassifierProp(core.class.Doc, 'name')).toBe('Doc') + expect(hierarchy.getClassifierProp(test.class.Task, 'name')).toBe('Task') + }) + + it('should handle property updates without creating new objects', async () => { + const hierarchy = prepare() + + // Set initial value + hierarchy.setClassifierProp(core.class.Space, 'counter', 0) + + // Update multiple times (testing that we're not creating new objects each time) + for (let i = 1; i <= 100; i++) { + hierarchy.setClassifierProp(core.class.Space, 'counter', i) + } + + // Verify final value + expect(hierarchy.getClassifierProp(core.class.Space, 'counter')).toBe(100) + }) + + // Edge cases and integration tests + it('should handle interface implementation checks correctly', async () => { + const hierarchy = prepare() + + // Task implements DummyWithState which extends WithState + expect(hierarchy.isImplements(test.class.Task, test.interface.WithState)).toBeTruthy() + expect(hierarchy.isImplements(test.class.Task, test.interface.DummyWithState)).toBeTruthy() + + // TaskCheckItem directly implements WithState + expect(hierarchy.isImplements(test.class.TaskCheckItem, test.interface.WithState)).toBeTruthy() + + // Negative cases + expect(hierarchy.isImplements(core.class.Space, test.interface.WithState)).toBeFalsy() + }) + + it('should maintain consistency after multiple hierarchy operations', async () => { + const hierarchy = prepare() + + // Perform multiple operations + const ancestors1 = hierarchy.getAncestors(test.class.Task) + const isDerived1 = hierarchy.isDerived(test.class.Task, core.class.Doc) + + // Set some properties + hierarchy.setClassifierProp(test.class.Task, 'test', 'value') + + // Verify operations still work correctly + const ancestors2 = hierarchy.getAncestors(test.class.Task) + const isDerived2 = hierarchy.isDerived(test.class.Task, core.class.Doc) + + expect(ancestors1).toEqual(ancestors2) + expect(isDerived1).toBe(isDerived2) + expect(isDerived2).toBeTruthy() + }) + + it('should handle getDescendants correctly', async () => { + const hierarchy = prepare() + + // Get descendants of Doc (should include many classes) + const descendants = hierarchy.getDescendants(core.class.Doc) + + expect(descendants).toContain(core.class.Space) + expect(descendants).toContain(core.class.Tx) + expect(descendants).toContain(test.class.Task) + expect(Array.isArray(descendants)).toBeTruthy() + }) + + it('should work with getBaseClass', async () => { + const hierarchy = prepare() + + // Get base class of a mixin + const baseClass = hierarchy.getBaseClass(test.mixin.TaskMixinTodos) + expect(baseClass).toBe(test.class.Task) + + // Get base class of a regular class (should return itself) + const baseClass2 = hierarchy.getBaseClass(test.class.Task) + expect(baseClass2).toBe(test.class.Task) + }) + + it('should handle getAllAttributes correctly', async () => { + const hierarchy = prepare() + + // Get all attributes for a class + const attributes = hierarchy.getAllAttributes(core.class.TxCreateDoc) + + // Should return a Map + expect(attributes instanceof Map).toBeTruthy() + + // Test with to parameter + const attributesTo = hierarchy.getAllAttributes(core.class.TxCreateDoc, core.class.Tx) + expect(attributesTo instanceof Map).toBeTruthy() + }) + + it('should maintain immutability of returned ancestors array', async () => { + const hierarchy = prepare() + + // Get ancestors + const ancestors = hierarchy.getAncestors(test.class.Task) + const originalLength = ancestors.length + + // The returned array should be the internal array, but modifying it shouldn't break hierarchy + // (This is a trade-off for memory optimization - callers should treat it as read-only) + expect(ancestors.length).toBe(originalLength) + expect(ancestors).toContain(core.class.Doc) + }) + + it('should handle performance for multiple isDerived checks', async () => { + const hierarchy = prepare() + + // Perform many isDerived checks to ensure array-based lookup is performant + const startTime = Date.now() + for (let i = 0; i < 1000; i++) { + hierarchy.isDerived(core.class.TxCreateDoc, core.class.Tx) + hierarchy.isDerived(test.class.Task, core.class.Doc) + hierarchy.isDerived(test.class.TaskCheckItem, core.class.AttachedDoc) + } + const endTime = Date.now() + + // Should complete in reasonable time (< 100ms for 3000 checks) + expect(endTime - startTime).toBeLessThan(100) + }) }) diff --git a/packages/core/src/hierarchy.ts b/packages/core/src/hierarchy.ts index 374dd85db0..9b8fc3665c 100644 --- a/packages/core/src/hierarchy.ts +++ b/packages/core/src/hierarchy.ts @@ -30,10 +30,10 @@ export class Hierarchy { private readonly attributes = new Map, Map>() private readonly attributesById = new Map, AnyAttribute>() private readonly descendants = new Map, Ref[]>() - private readonly ancestors = new Map, Set>>() + private readonly ancestors = new Map, Ref[]>() private readonly proxies = new Map>, ProxyHandler>() - private readonly classifierProperties = new Map, Record>() + private readonly classifierProperties = new Map, Map>() private createMixinProxyHandler (mixin: Ref>): ProxyHandler { const value = this.getClass(mixin) @@ -172,7 +172,7 @@ export class Hierarchy { if (result === undefined) { throw new Error('ancestors not found: ' + _class) } - return Array.from(result) + return result } getClass(_class: Ref>): Class { @@ -318,7 +318,7 @@ export class Hierarchy { * It will iterate over parents. */ isDerived(_class: Ref>, from: Ref>): boolean { - return this.ancestors.get(_class)?.has(from) ?? false + return this.ancestors.get(_class)?.includes(from) ?? false } /** @@ -408,29 +408,21 @@ export class Hierarchy { private updateAncestors (_class: Ref, add = true): void { const cl: Ref[] = [_class] const visited = new Set>() + const ancestorList: Ref[] = [] + while (cl.length > 0) { const classifier = cl.shift() as Ref if (addNew(visited, classifier)) { - const list = this.ancestors.get(_class) - if (list === undefined) { - if (add) { - this.ancestors.set(_class, new Set([classifier])) - } - } else { - if (add) { - if (!list.has(classifier)) { - list.add(classifier) - } - } else { - const pos = list.has(classifier) - if (pos) { - list.delete(classifier) - } - } - } + ancestorList.push(classifier) cl.push(...this.ancestorsOf(classifier)) } } + + if (add) { + this.ancestors.set(_class, ancestorList) + } else { + this.ancestors.delete(_class) + } } /** @@ -623,12 +615,16 @@ export class Hierarchy { } getClassifierProp (cl: Ref>, prop: string): any | undefined { - return this.classifierProperties.get(cl)?.[prop] + return this.classifierProperties.get(cl)?.get(prop) } setClassifierProp (cl: Ref>, prop: string, value: any): void { - const cur = this.classifierProperties.get(cl) - this.classifierProperties.set(cl, { ...cur, [prop]: value }) + let cur = this.classifierProperties.get(cl) + if (cur === undefined) { + cur = new Map() + this.classifierProperties.set(cl, cur) + } + cur.set(prop, value) } }