Skip to content

Commit 5d38e47

Browse files
authored
chore: optimised isChildPropertyPath to not use a regex and added more logging around calculateSubTreeSortOrder (#41162)
## Description Reduced the cumulative contribution of isChildPropertyPath by approximately 98%. During page load, it originally took around 100 ms for a customer app on a Mac machine and is now down to 2 ms. As a result, calculateSubTreeSortOrder has improved by 70% on the same setup. Optimised sorting and removed redundant lookups in addNodes, which led to marginal gains. This optimisation specifically targets a customer scenario where addNodes and addDependantsOfNestedPropertyPaths are heavily stressed, contributing to an overall latency of about 7 seconds. Added additional logging to investigate the issue further. 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/16714192460> > Commit: d6633bb > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=16714192460&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Mon, 04 Aug 2025 05:57:55 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** * Added a new function to improve detection of child property paths supporting both dot and bracket notation. * **Refactor** * Optimized internal logic for managing dependency sets and improved node addition efficiency. * Updated sorting method to accept arrays for better consistency. * **Style** * Enhanced code readability and maintainability with more concise patterns. * **Chores** * Introduced performance timing and logging for key operations to aid in monitoring and diagnostics. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent d0b8899 commit 5d38e47

4 files changed

Lines changed: 100 additions & 28 deletions

File tree

app/client/src/ce/workers/Evaluation/evaluationUtils.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -359,11 +359,10 @@ export const addDependantsOfNestedPropertyPaths = (
359359
parentPaths: Array<string>,
360360
inverseMap: DependencyMap,
361361
): Set<string> => {
362-
const withNestedPaths: Set<string> = new Set();
362+
const withNestedPaths: Set<string> = new Set(parentPaths);
363363
const dependantNodes = Object.keys(inverseMap);
364364

365365
parentPaths.forEach((propertyPath) => {
366-
withNestedPaths.add(propertyPath);
367366
dependantNodes
368367
.filter((dependantNodePath) =>
369368
isChildPropertyPath(propertyPath, dependantNodePath),

app/client/src/entities/DependencyMap/index.ts

Lines changed: 37 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -82,19 +82,26 @@ export default class DependencyMap {
8282
if (this.#nodes.has(dependency)) {
8383
validDependencies.add(dependency);
8484

85-
if (this.#dependenciesInverse.has(dependency)) {
86-
this.#dependenciesInverse.get(dependency)?.add(node);
87-
} else {
88-
this.#dependenciesInverse.set(dependency, new Set([node]));
85+
let inverseSet = this.#dependenciesInverse.get(dependency);
86+
87+
if (!inverseSet) {
88+
inverseSet = new Set();
89+
this.#dependenciesInverse.set(dependency, inverseSet);
8990
}
91+
92+
inverseSet.add(node);
9093
} else {
9194
invalidDependencies.add(dependency);
9295

93-
if (this.#invalidDependenciesInverse.has(dependency)) {
94-
this.#invalidDependenciesInverse.get(dependency)?.add(node);
95-
} else {
96-
this.#invalidDependenciesInverse.set(dependency, new Set([node]));
96+
let inverseInvalidSet =
97+
this.#invalidDependenciesInverse.get(dependency);
98+
99+
if (!inverseInvalidSet) {
100+
inverseInvalidSet = new Set();
101+
this.#invalidDependenciesInverse.set(dependency, inverseInvalidSet);
97102
}
103+
104+
inverseInvalidSet.add(node);
98105
}
99106
}
100107

@@ -163,10 +170,9 @@ export default class DependencyMap {
163170
* @param nodes Record of nodes to sort
164171
* @returns Array of node keys sorted by depth
165172
*/
166-
private sortNodesByDepth = (nodes: Record<string, true>): string[] => {
173+
private sortNodesByDepth = (nodes: string[]): string[] => {
167174
// Pre-compute depths for all nodes
168-
const nodeKeys = Object.keys(nodes);
169-
const nodeDepths = nodeKeys.map((node) => ({
175+
const nodeDepths = nodes.map((node) => ({
170176
node,
171177
depth: node.split(".").length,
172178
}));
@@ -186,15 +192,16 @@ export default class DependencyMap {
186192
*/
187193

188194
addNodes = (nodes: Record<string, true>, strict = true) => {
195+
const newUnaddedNodes = Object.keys(nodes).filter(
196+
(node) => !this.#nodes.has(node),
197+
);
189198
const nodesToAdd = strict
190-
? Object.keys(nodes)
191-
: this.sortNodesByDepth(nodes);
199+
? newUnaddedNodes
200+
: this.sortNodesByDepth(newUnaddedNodes);
192201

193202
let didUpdateGraph = false;
194203

195204
for (const newNode of nodesToAdd) {
196-
if (this.#nodes.has(newNode)) continue;
197-
198205
// New node introduced to the graph.
199206
this.#nodes.set(newNode, true);
200207
// Check the paths that consumed this node before it was introduced.
@@ -228,11 +235,15 @@ export default class DependencyMap {
228235
this.#invalidDependencies.get(iNode)?.delete(newNode);
229236
didUpdateGraph = true;
230237

231-
if (this.#dependenciesInverse.has(newNode)) {
232-
this.#dependenciesInverse.get(newNode)?.add(iNode);
233-
} else {
234-
this.#dependenciesInverse.set(newNode, new Set([iNode]));
238+
// Get or create the inverse dependencies set for the new node
239+
let inverseSet = this.#dependenciesInverse.get(newNode);
240+
241+
if (!inverseSet) {
242+
inverseSet = new Set();
243+
this.#dependenciesInverse.set(newNode, inverseSet);
235244
}
245+
246+
inverseSet.add(iNode);
236247
}
237248

238249
this.#invalidDependenciesInverse.delete(newNode);
@@ -261,11 +272,14 @@ export default class DependencyMap {
261272

262273
if (!this.#nodes.has(iNode)) continue;
263274

264-
if (this.#invalidDependenciesInverse.has(node)) {
265-
this.#invalidDependenciesInverse.get(node)?.add(iNode);
266-
} else {
267-
this.#invalidDependenciesInverse.set(node, new Set([iNode]));
275+
let inverseInvalidSet = this.#invalidDependenciesInverse.get(node);
276+
277+
if (!inverseInvalidSet) {
278+
inverseInvalidSet = new Set();
279+
this.#invalidDependenciesInverse.set(node, inverseInvalidSet);
268280
}
281+
282+
inverseInvalidSet.add(iNode);
269283
}
270284

271285
this.#dependenciesInverse.delete(node);

app/client/src/utils/DynamicBindingUtils.ts

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,46 @@ export const unsafeFunctionForEval = [
299299
"Navigator",
300300
];
301301

302+
/**
303+
* Checks if a child property path starts with the parent property path
304+
* using either dot notation (.) or bracket notation ([)
305+
*
306+
* @param parentPropertyPath - The parent property path
307+
* @param childPropertyPath - The child property path to check
308+
* @returns true if childPropertyPath is a child of parentPropertyPath
309+
*
310+
* @example
311+
* isChildPropertyPathStartsWithParent("Table1", "Table1.data") // true
312+
* isChildPropertyPathStartsWithParent("Table1", "Table1[0]") // true
313+
* isChildPropertyPathStartsWithParent("Table1", "Table2.data") // false
314+
*/
315+
export const isChildPropertyPathStartsWithParent = (
316+
parentPropertyPath: string,
317+
childPropertyPath: string,
318+
): boolean => {
319+
if (!parentPropertyPath || !childPropertyPath) {
320+
return false;
321+
}
322+
323+
const parentLength = parentPropertyPath.length;
324+
325+
if (childPropertyPath.length <= parentLength) {
326+
return false;
327+
}
328+
329+
// Most common case: dot notation
330+
if (childPropertyPath[parentLength] === ".") {
331+
return childPropertyPath.startsWith(parentPropertyPath);
332+
}
333+
334+
// Less common case: bracket notation
335+
if (childPropertyPath[parentLength] === "[") {
336+
return childPropertyPath.startsWith(parentPropertyPath);
337+
}
338+
339+
return false;
340+
};
341+
302342
export const isChildPropertyPath = (
303343
parentPropertyPath: string,
304344
childPropertyPath: string,
@@ -308,8 +348,7 @@ export const isChildPropertyPath = (
308348
): boolean => {
309349
return (
310350
(!strict && parentPropertyPath === childPropertyPath) ||
311-
childPropertyPath.startsWith(`${parentPropertyPath}.`) ||
312-
childPropertyPath.startsWith(`${parentPropertyPath}[`)
351+
isChildPropertyPathStartsWithParent(parentPropertyPath, childPropertyPath)
313352
);
314353
};
315354

app/client/src/workers/common/DataTreeEvaluator/index.ts

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1958,6 +1958,8 @@ export default class DataTreeEvaluator {
19581958
const changePaths: Set<string> = new Set(dependenciesOfRemovedPaths);
19591959
const configTree = this.getConfigTree();
19601960

1961+
const updatedValuePathsLatencyStart = performance.now();
1962+
19611963
for (const pathArray of updatedValuePaths) {
19621964
const fullPropertyPath = convertPathToString(pathArray);
19631965

@@ -2005,27 +2007,45 @@ export default class DataTreeEvaluator {
20052007
}
20062008
}
20072009

2010+
const updatedValuePathsLatency =
2011+
performance.now() - updatedValuePathsLatencyStart;
2012+
20082013
// If a nested property path has changed and someone (say x) is dependent on the parent of the said property,
20092014
// x must also be evaluated. For example, the following relationship exists in dependency map:
20102015
// < "Input1.defaultText" : ["Table1.selectedRow.email"] >
20112016
// If Table1.selectedRow has changed, then Input1.defaultText must also be evaluated because Table1.selectedRow.email
20122017
// is a nested property of Table1.selectedRow
2018+
const addDependantsOfNestedPropertyPathsLatencyStart = performance.now();
20132019
const changePathsWithNestedDependants = addDependantsOfNestedPropertyPaths(
20142020
Array.from(changePaths),
20152021
this.inverseDependencies,
20162022
);
2017-
2023+
const addDependantsOfNestedPropertyPathsLatency =
2024+
performance.now() - addDependantsOfNestedPropertyPathsLatencyStart;
2025+
const trimDependantChangePathsLatencyStart = performance.now();
20182026
const trimmedChangedPaths = trimDependantChangePaths(
20192027
changePathsWithNestedDependants,
20202028
this.dependencies,
20212029
);
2030+
const trimDependantChangePathsLatency =
2031+
performance.now() - trimDependantChangePathsLatencyStart;
20222032

20232033
// Now that we have all the root nodes which have to be evaluated, recursively find all the other paths which
20242034
// would get impacted because they are dependent on the said root nodes and add them in order
2035+
const completeSortOrderLatencyStart = performance.now();
20252036
const completeSortOrder = this.getCompleteSortOrder(
20262037
trimmedChangedPaths,
20272038
this.inverseDependencies,
20282039
);
2040+
const completeSortOrderLatency =
2041+
performance.now() - completeSortOrderLatencyStart;
2042+
2043+
this.logs.push({
2044+
updatedValuePathsLatency,
2045+
addDependantsOfNestedPropertyPathsLatency,
2046+
trimDependantChangePathsLatency,
2047+
completeSortOrderLatency,
2048+
});
20292049

20302050
// Remove any paths that do not exist in the data tree anymore
20312051
return difference(

0 commit comments

Comments
 (0)