Protei/PromucFlow_constructor
2
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

chore: allKeys computation is constrained by diff (#35303)

Browse Source 
## 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"

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/10220298181>
> Commit: 12b94c33ecfd3fb069b003df16a0f3192f769d62
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10220298181&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.All`
> Spec:
> <hr>Fri, 02 Aug 2024 19:22:32 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [ ] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## 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.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
Vemparala Surya Vamsi 2024-08-03 14:06:16 +05:30 committed by GitHub
parent 6578bdea74
commit 964fb0e1aa
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 172 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
app/client/src/ce/workers/Evaluation/evaluationUtils.ts
View File

@ -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<string, unknown> | unknown,
curKey = "",
result: Record<string, true> = {},
): Record<string, true> => {
@ -535,6 +533,27 @@ export const getAllPaths = (
}
return result;
};
export const getAllPathsBasedOnDiffPaths = (
records: Record<string, unknown> | unknown,
diff: DataTreeDiff[],
// this argument would be mutable
previousResult: Record<string, true> = {},
): Record<string, true> => {
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<string>,
dependencyMap: DependencyMap,

11
app/client/src/workers/common/DataTreeEvaluator/index.ts
View File

@ -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

142
app/client/src/workers/common/DataTreeEvaluator/utils.test.ts
View File

@ -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<string, unknown>,
};
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<string, true>;
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);
});
});