Skip to content

Commit

Permalink
chore: makeParentDependedOnChildren optimisation only affected nodes …
Browse files Browse the repository at this point in the history
…in the graph is computed (appsmithorg#36161)

## Description
Optimising `makeParentDependedOnChildren` function by executing
`makeParentsDependOnChild` for only affected nodes in the dependencyMap
graph. This has brought down the cumulative latency of execution by
about 90% when testing it against a customer's app during page load.

Fixes #`Issue Number`  
_or_  
Fixes `Issue URL`
> [!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/10791040934>
> Commit: 902f465
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10791040934&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.All`
> Spec:
> <hr>Tue, 10 Sep 2024 12:20:44 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

- **New Features**
- Introduced a method for linking child nodes to parent nodes in the
dependency graph, allowing for more precise management of affected
nodes.
- Added new helper functions to streamline the addition of affected
nodes and dependencies within the dependency map.

- **Bug Fixes**
- Enhanced testing coverage for the dependency linking logic, ensuring
accurate representation of hierarchical relationships between nodes.

- **Documentation**
- Improved comments in existing methods to clarify functionality and
enhance code readability.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
vsvamsi1 authored and Shivam-z committed Sep 26, 2024
1 parent 70c1e39 commit 78a2d2a
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 12 deletions.
21 changes: 20 additions & 1 deletion app/client/src/entities/DependencyMap/DependencyMapUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export class DependencyMapUtils {
return { success: false, cyclicNode: node, error };
}
}

// this function links childNode to its parent as a dependency for the entire dependencyGraph
static makeParentsDependOnChildren(dependencyMap: DependencyMap) {
const dependencies = dependencyMap.rawDependencies;
for (const [node, deps] of dependencies.entries()) {
Expand All @@ -49,6 +49,25 @@ export class DependencyMapUtils {
return dependencyMap;
}

// this function links childNode to its parent as a dependency for only affectedNodes in the graph
static linkAffectedChildNodesToParent(
dependencyMap: DependencyMap,
affectedSet: Set<string>,
) {
const dependencies = dependencyMap.rawDependencies;
for (const [node, deps] of dependencies.entries()) {
if (affectedSet.has(node)) {
DependencyMapUtils.makeParentsDependOnChild(dependencyMap, node);
}
deps.forEach((dep) => {
if (affectedSet.has(dep)) {
DependencyMapUtils.makeParentsDependOnChild(dependencyMap, dep);
}
});
}
return dependencyMap;
}

static makeParentsDependOnChild = (
dependencyMap: DependencyMap,
child: string,
Expand Down
64 changes: 64 additions & 0 deletions app/client/src/entities/DependencyMap/__tests__/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2708,6 +2708,70 @@ describe("Tests for DependencyMapUtils", () => {
"Api1",
]);
}
describe("linkAffectedChildNodesToParent", () => {
const createSomeDependencyMap = () => {
const dependencyMap = new DependencyMap();
dependencyMap.addNodes({
tableWidget: true,
apiData: true,
"tableWidget.tableData": true,
"apiData.data": true,
"apiData.allData": true,
"apiData.allData.paginatedData": true,
});
dependencyMap.addDependency("tableWidget", ["tableWidget.tableData"]);
return dependencyMap;
};

test("should link child node to its parentNode as a dependant", () => {
const dependencyMap = createSomeDependencyMap();
dependencyMap.addDependency("tableWidget.tableData", ["apiData.data"]);
// although "apiData.data" was attached as a dependency to "tableWidget.tableData", it's parent "apiData" also needs to be linked to "apiData.data"
expect(dependencyMap.rawDependencies.get("apiData")).toEqual(undefined);
const affectedNodes = new Set(["apiData.data"]);
DependencyMapUtils.linkAffectedChildNodesToParent(
dependencyMap,
affectedNodes,
);
// after linkAffectedChildNodesToParent execution "apiData" does get linked to "apiData.data"
expect(dependencyMap.rawDependencies.get("apiData")).toEqual(
new Set(["apiData.data"]),
);
});
test("should not link child node to its parentNode as a dependant when the child node is not affected ", () => {
const dependencyMap = createSomeDependencyMap();
dependencyMap.addDependency("tableWidget.tableData", ["apiData.data"]);
// although "apiData.data" was attached as a dependency to "tableWidget.tableData", it's parent "apiData" also needs to be linked to "apiData.data"
expect(dependencyMap.rawDependencies.get("apiData")).toEqual(undefined);
const emptyAffectedNodes = new Set([]);
DependencyMapUtils.linkAffectedChildNodesToParent(
dependencyMap,
emptyAffectedNodes,
);
expect(dependencyMap.rawDependencies.get("apiData")).toEqual(undefined);
});

test("should link child node to its grand parent node as a dependant", () => {
const dependencyMap = createSomeDependencyMap();
dependencyMap.addDependency("tableWidget.tableData", [
"apiData.allData.paginatedData",
]);
expect(dependencyMap.rawDependencies.get("apiData")).toEqual(undefined);
const affectedNodes = new Set(["apiData.allData.paginatedData"]);

DependencyMapUtils.linkAffectedChildNodesToParent(
dependencyMap,
affectedNodes,
);
// after linkAffectedChildNodesToParent execution "apiData.allData.paginatedData" get linked to "apiData.data" and its parent "apiData.data" gets linked to "apiData"
expect(dependencyMap.getDirectDependencies("apiData")).toEqual([
"apiData.allData",
]);
expect(dependencyMap.getDirectDependencies("apiData.allData")).toEqual([
"apiData.allData.paginatedData",
]);
});
});
describe("makeParentsDependOnChild", () => {
const createSomeDependencyMap = () => {
const dependencyMap = new DependencyMap();
Expand Down
66 changes: 55 additions & 11 deletions app/client/src/workers/common/DependencyMap/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
} from "ee/workers/Evaluation/Actions";
import { isWidgetActionOrJsObject } from "ee/entities/DataTree/utils";
import { getValidEntityType } from "workers/common/DataTreeEvaluator/utils";
import type DependencyMap from "entities/DependencyMap";

export function createDependencyMap(
dataTreeEvalRef: DataTreeEvaluator,
Expand Down Expand Up @@ -72,6 +73,30 @@ export function createDependencyMap(
inverseDependencies: dependencyMap.inverseDependencies,
};
}
const addingAffectedNodesToList = (
affectedNodes: Set<string>,
addedNodes: string[],
) => {
addedNodes.forEach((v) => affectedNodes.add(v));
};

const addNodesToDependencyMap =
(affectedNodes: Set<string>, dependencyMap: DependencyMap) =>
(addedNodes: Record<string, true>, strict?: boolean) => {
const didUpdateDep = dependencyMap.addNodes(addedNodes, strict);
if (didUpdateDep) {
addingAffectedNodesToList(affectedNodes, Object.keys(addedNodes));
}
return didUpdateDep;
};

const setDependenciesToDependencyMap =
(affectedNodes: Set<string>, dependencyMap: DependencyMap) =>
(node: string, dependencies: string[]) => {
dependencyMap.addDependency(node, dependencies);

addingAffectedNodesToList(affectedNodes, [node, ...dependencies]);
};

export const updateDependencyMap = ({
configTree,
Expand All @@ -91,7 +116,15 @@ export const updateDependencyMap = ({
const { allKeys, dependencyMap, oldConfigTree, oldUnEvalTree } =
dataTreeEvalRef;
let { errors: dataTreeEvalErrors } = dataTreeEvalRef;

const affectedNodes: Set<string> = new Set();
const addNodesToDepedencyMapFn = addNodesToDependencyMap(
affectedNodes,
dependencyMap,
);
const setDependenciesToDepedencyMapFn = setDependenciesToDependencyMap(
affectedNodes,
dependencyMap,
);
translatedDiffs.forEach((dataTreeDiff) => {
const {
event,
Expand All @@ -117,15 +150,18 @@ export const updateDependencyMap = ({
});
// If a new entity is added, add setter functions to all nodes
if (entityName === fullPropertyPath) {
const didUpdateDep = dependencyMap.addNodes(
getEntitySetterFunctions(entityConfig, entityName, entity),
const addedNodes = getEntitySetterFunctions(
entityConfig,
entityName,
entity,
);
if (didUpdateDep) didUpdateDependencyMap = true;
didUpdateDependencyMap =
addNodesToDepedencyMapFn(addedNodes) || didUpdateDependencyMap;
}

const didUpdateDep = dependencyMap.addNodes(allAddedPaths, false);

if (didUpdateDep) didUpdateDependencyMap = true;
didUpdateDependencyMap =
addNodesToDepedencyMapFn(allAddedPaths, false) ||
didUpdateDependencyMap;

if (isWidgetActionOrJsObject(entity)) {
if (!isDynamicLeaf(unEvalDataTree, fullPropertyPath, configTree)) {
Expand All @@ -141,7 +177,9 @@ export const updateDependencyMap = ({
([path, pathDependencies]) => {
const { errors: extractDependencyErrors, references } =
extractInfoFromBindings(pathDependencies, allKeys);
dependencyMap.addDependency(path, references);

setDependenciesToDepedencyMapFn(path, references);

didUpdateDependencyMap = true;
dataTreeEvalErrors = dataTreeEvalErrors.concat(
extractDependencyErrors,
Expand All @@ -158,7 +196,9 @@ export const updateDependencyMap = ({
);
const { errors: extractDependencyErrors, references } =
extractInfoFromBindings(entityPathDependencies, allKeys);
dependencyMap.addDependency(fullPropertyPath, references);

setDependenciesToDepedencyMapFn(fullPropertyPath, references);

didUpdateDependencyMap = true;
dataTreeEvalErrors = dataTreeEvalErrors.concat(
extractDependencyErrors,
Expand Down Expand Up @@ -218,7 +258,8 @@ export const updateDependencyMap = ({
);
const { errors: extractDependencyErrors, references } =
extractInfoFromBindings(entityPathDependencies, allKeys);
dependencyMap.addDependency(fullPropertyPath, references);
setDependenciesToDepedencyMapFn(fullPropertyPath, references);

didUpdateDependencyMap = true;
dataTreeEvalErrors = dataTreeEvalErrors.concat(
extractDependencyErrors,
Expand All @@ -237,7 +278,10 @@ export const updateDependencyMap = ({
const updateChangedDependenciesStart = performance.now();

if (didUpdateDependencyMap) {
DependencyMapUtils.makeParentsDependOnChildren(dependencyMap);
DependencyMapUtils.linkAffectedChildNodesToParent(
dependencyMap,
affectedNodes,
);
dataTreeEvalRef.sortedDependencies = dataTreeEvalRef.sortDependencies(
dependencyMap,
translatedDiffs,
Expand Down

0 comments on commit 78a2d2a

Please sign in to comment.