From 964fb0e1aa8a35b299d960bb02442a136db32030 Mon Sep 17 00:00:00 2001 From: Vemparala Surya Vamsi <121419957+vsvamsi1@users.noreply.github.com> Date: Sat, 3 Aug 2024 14:06:16 +0530 Subject: [PATCH] chore: allKeys computation is constrained by diff (#35303) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description allKeys is previously computed from the complete unevalTree, we used to recursively traverse through the entire unevalTree during each evaluation update cycle. We are optimising this by leveraging the diff which we have previously computed and using the diff to directly update the allKeys result. When delete based diffs detected are detected we are directly deleting the nodes in the previous allKeys result, for only new nodes we are recursively computing the allKeys for those nodes and merging it back to the previous allKeys result. The time complexity of the solution has improved by limiting iterations to the diff alone instead of computing the entire unevalTree. Fixes #35386 > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.All" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: 12b94c33ecfd3fb069b003df16a0f3192f769d62 > Cypress dashboard. > Tags: `@tag.All` > Spec: >
Fri, 02 Aug 2024 19:22:32 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Introduced a new function for dynamic path management based on differential updates. - Enhanced data processing in the DataTreeEvaluator class to improve dependency tracking and efficiency. - **Bug Fixes** - Improved handling of data tree updates to ensure accurate addition and deletion of paths. - **Tests** - Added a comprehensive test suite for the new path management function, verifying accurate response to various data tree changes. --- .../ce/workers/Evaluation/evaluationUtils.ts | 25 ++- .../workers/common/DataTreeEvaluator/index.ts | 11 +- .../common/DataTreeEvaluator/utils.test.ts | 142 +++++++++++++++++- 3 files changed, 172 insertions(+), 6 deletions(-) diff --git a/app/client/src/ce/workers/Evaluation/evaluationUtils.ts b/app/client/src/ce/workers/Evaluation/evaluationUtils.ts index c6e373c506a5..880011f0eed3 100644 --- a/app/client/src/ce/workers/Evaluation/evaluationUtils.ts +++ b/app/client/src/ce/workers/Evaluation/evaluationUtils.ts @@ -514,9 +514,7 @@ export const getImmediateParentsOfPropertyPaths = ( }; export const getAllPaths = ( - // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - records: any, + records: Record | unknown, curKey = "", result: Record = {}, ): Record => { @@ -535,6 +533,27 @@ export const getAllPaths = ( } return result; }; +export const getAllPathsBasedOnDiffPaths = ( + records: Record | unknown, + diff: DataTreeDiff[], + // this argument would be mutable + previousResult: Record = {}, +): Record => { + const newResult = previousResult; + diff.forEach((curr) => { + const { event, payload } = curr; + if (event === DataTreeDiffEvent.DELETE) { + delete newResult[payload.propertyPath]; + } + if (event === DataTreeDiffEvent.NEW || event === DataTreeDiffEvent.EDIT) { + const newDataSegments = get(records, payload.propertyPath); + // directly mutates on the result so we don't have to merge it back to the result + getAllPaths(newDataSegments, payload.propertyPath, newResult); + } + }); + + return newResult; +}; export const trimDependantChangePaths = ( changePaths: Set, dependencyMap: DependencyMap, diff --git a/app/client/src/workers/common/DataTreeEvaluator/index.ts b/app/client/src/workers/common/DataTreeEvaluator/index.ts index a6675900f765..cec156f441a6 100644 --- a/app/client/src/workers/common/DataTreeEvaluator/index.ts +++ b/app/client/src/workers/common/DataTreeEvaluator/index.ts @@ -35,7 +35,10 @@ import type { import { EvaluationSubstitutionType } from "entities/DataTree/dataTreeFactory"; import { ENTITY_TYPE } from "@appsmith/entities/DataTree/types"; import type { DataTreeDiff } from "@appsmith/workers/Evaluation/evaluationUtils"; -import { convertMicroDiffToDeepDiff } from "@appsmith/workers/Evaluation/evaluationUtils"; +import { + convertMicroDiffToDeepDiff, + getAllPathsBasedOnDiffPaths, +} from "@appsmith/workers/Evaluation/evaluationUtils"; import { addDependantsOfNestedPropertyPaths, @@ -662,7 +665,11 @@ export default class DataTreeEvaluator { // TODO => Optimize using dataTree diff this.allKeys = profileFn("getAllPaths", undefined, webworkerTelemetry, () => - getAllPaths(unEvalTreeWithStringifiedJSFunctions), + getAllPathsBasedOnDiffPaths( + unEvalTreeWithStringifiedJSFunctions, + translatedDiffs, + this.allKeys, + ), ); // Find all the paths that have changed as part of the difference and update the // global dependency map if an existing dynamic binding has now become legal diff --git a/app/client/src/workers/common/DataTreeEvaluator/utils.test.ts b/app/client/src/workers/common/DataTreeEvaluator/utils.test.ts index e956570f6340..c10ed8e5f643 100644 --- a/app/client/src/workers/common/DataTreeEvaluator/utils.test.ts +++ b/app/client/src/workers/common/DataTreeEvaluator/utils.test.ts @@ -4,7 +4,12 @@ import type { } from "@appsmith/entities/DataTree/types"; import { getOnlyAffectedJSObjects, getIsNewWidgetAdded } from "./utils"; import type { UnEvalTree } from "entities/DataTree/dataTreeTypes"; -import type { DataTreeDiff } from "@appsmith/workers/Evaluation/evaluationUtils"; +import { + DataTreeDiffEvent, + getAllPathsBasedOnDiffPaths, + type DataTreeDiff, +} from "@appsmith/workers/Evaluation/evaluationUtils"; +import produce from "immer"; describe("getOnlyAffectedJSObjects", () => { const dataTree = { @@ -100,3 +105,138 @@ describe("getIsNewWidgetAdded", () => { expect(result).toBeFalsy(); }); }); + +describe("getAllPathsBasedOnDiffPaths", () => { + const initialTree = { + WidgetName: { + 1: "yo", + name: "WidgetName", + objectProperty: { + childObjectProperty: [ + "1", + 1, + { + key: "value", + 2: 1, + }, + ["1", "2"], + ], + }, + stringProperty: new String("Hello"), + } as Record, + }; + const initialAllKeys = { + WidgetName: true, + "WidgetName.1": true, + "WidgetName.name": true, + "WidgetName.objectProperty": true, + "WidgetName.objectProperty.childObjectProperty": true, + "WidgetName.objectProperty.childObjectProperty[0]": true, + "WidgetName.objectProperty.childObjectProperty[1]": true, + "WidgetName.objectProperty.childObjectProperty[2]": true, + "WidgetName.objectProperty.childObjectProperty[2].key": true, + "WidgetName.objectProperty.childObjectProperty[2].2": true, + "WidgetName.objectProperty.childObjectProperty[3]": true, + "WidgetName.objectProperty.childObjectProperty[3][0]": true, + "WidgetName.objectProperty.childObjectProperty[3][1]": true, + "WidgetName.stringProperty": true, + } as Record; + + test("should generate all paths of the widget when a new widget as added", () => { + const updatedAllKeys = getAllPathsBasedOnDiffPaths( + initialTree, + [ + { + event: DataTreeDiffEvent.NEW, + payload: { propertyPath: "WidgetName" }, + }, + ], + // allKeys is empty initally + {}, + ); + expect(initialAllKeys).toEqual(updatedAllKeys); + }); + test("should not update allKeys when there are no diffs", () => { + const updatedAllKeys = getAllPathsBasedOnDiffPaths(initialTree, [], { + ...initialAllKeys, + }); + // allkeys are not altered here since the diff is empty + expect(initialAllKeys).toEqual(updatedAllKeys); + }); + test("should delete the correct paths within allKeys when a node within a widget is deleted", () => { + const deletedWidgetName = produce(initialTree, (draft) => { + // a property within the widget is deleted + delete draft.WidgetName.name; + }); + const updatedAllKeys = getAllPathsBasedOnDiffPaths( + deletedWidgetName, + [ + { + event: DataTreeDiffEvent.DELETE, + payload: { + propertyPath: "WidgetName.name", + }, + }, + ], + // we have to make a copy since allKeys is mutable + { ...initialAllKeys }, + ); + const deletedWidgetNameInAllKeys = produce(initialAllKeys, (draft) => { + delete draft["WidgetName.name"]; + }); + + expect(deletedWidgetNameInAllKeys).toEqual(updatedAllKeys); + }); + + test("should add the correct paths to the allKeys when a node within a widget is added", () => { + const addedNewWidgetProperty = produce(initialTree, (draft) => { + // new property is added to the widget + draft.WidgetName.widgetNewProperty = "newValue"; + }); + + const updatedAllKeys = getAllPathsBasedOnDiffPaths( + addedNewWidgetProperty, + [ + { + event: DataTreeDiffEvent.NEW, + payload: { + propertyPath: "WidgetName.widgetNewProperty", + }, + }, + ], + // we have to make a copy since allKeys is mutable + { ...initialAllKeys }, + ); + const addedNewWidgetPropertyInAllKeys = produce(initialAllKeys, (draft) => { + draft["WidgetName.widgetNewProperty"] = true; + }); + + expect(addedNewWidgetPropertyInAllKeys).toEqual(updatedAllKeys); + }); + + test("should generate the correct paths when the value changes form a simple primitive to a collection, this is for EDIT diffs", () => { + const addedNewWidgetProperty = produce(initialTree, (draft) => { + //existing property within the widget is edited + draft.WidgetName.name = [{ a: 1 }]; + }); + + const updatedAllKeys = getAllPathsBasedOnDiffPaths( + addedNewWidgetProperty, + [ + { + event: DataTreeDiffEvent.EDIT, + payload: { + propertyPath: "WidgetName.name", + }, + }, + ], + // we have to make a copy since allKeys is mutable + { ...initialAllKeys }, + ); + const addedACollectionInAllKeys = produce(initialAllKeys, (draft) => { + draft["WidgetName.name[0]"] = true; + draft["WidgetName.name[0].a"] = true; + }); + expect(addedACollectionInAllKeys).toEqual(updatedAllKeys); + }); +});