Skip to content

Commit

Permalink
chore: allKeys computation is constrained by diff (#35303)
Browse files Browse the repository at this point in the history
## 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: 12b94c3
> <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 -->
  • Loading branch information
vsvamsi1 authored Aug 3, 2024
1 parent 6578bde commit 964fb0e
Show file tree
Hide file tree
Showing 3 changed files with 172 additions and 6 deletions.
25 changes: 22 additions & 3 deletions app/client/src/ce/workers/Evaluation/evaluationUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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> => {
Expand All @@ -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,
Expand Down
11 changes: 9 additions & 2 deletions app/client/src/workers/common/DataTreeEvaluator/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
142 changes: 141 additions & 1 deletion app/client/src/workers/common/DataTreeEvaluator/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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);
});
});

0 comments on commit 964fb0e

Please sign in to comment.