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: d6633bb07190c897a9a9d9563e606c4dd220fa55 > <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 -->
This commit is contained in:
parent
d0b88994aa
commit
5d38e47508
|
|
@ -359,11 +359,10 @@ export const addDependantsOfNestedPropertyPaths = (
|
||||||
parentPaths: Array<string>,
|
parentPaths: Array<string>,
|
||||||
inverseMap: DependencyMap,
|
inverseMap: DependencyMap,
|
||||||
): Set<string> => {
|
): Set<string> => {
|
||||||
const withNestedPaths: Set<string> = new Set();
|
const withNestedPaths: Set<string> = new Set(parentPaths);
|
||||||
const dependantNodes = Object.keys(inverseMap);
|
const dependantNodes = Object.keys(inverseMap);
|
||||||
|
|
||||||
parentPaths.forEach((propertyPath) => {
|
parentPaths.forEach((propertyPath) => {
|
||||||
withNestedPaths.add(propertyPath);
|
|
||||||
dependantNodes
|
dependantNodes
|
||||||
.filter((dependantNodePath) =>
|
.filter((dependantNodePath) =>
|
||||||
isChildPropertyPath(propertyPath, dependantNodePath),
|
isChildPropertyPath(propertyPath, dependantNodePath),
|
||||||
|
|
|
||||||
|
|
@ -82,19 +82,26 @@ export default class DependencyMap {
|
||||||
if (this.#nodes.has(dependency)) {
|
if (this.#nodes.has(dependency)) {
|
||||||
validDependencies.add(dependency);
|
validDependencies.add(dependency);
|
||||||
|
|
||||||
if (this.#dependenciesInverse.has(dependency)) {
|
let inverseSet = this.#dependenciesInverse.get(dependency);
|
||||||
this.#dependenciesInverse.get(dependency)?.add(node);
|
|
||||||
} else {
|
if (!inverseSet) {
|
||||||
this.#dependenciesInverse.set(dependency, new Set([node]));
|
inverseSet = new Set();
|
||||||
|
this.#dependenciesInverse.set(dependency, inverseSet);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
inverseSet.add(node);
|
||||||
} else {
|
} else {
|
||||||
invalidDependencies.add(dependency);
|
invalidDependencies.add(dependency);
|
||||||
|
|
||||||
if (this.#invalidDependenciesInverse.has(dependency)) {
|
let inverseInvalidSet =
|
||||||
this.#invalidDependenciesInverse.get(dependency)?.add(node);
|
this.#invalidDependenciesInverse.get(dependency);
|
||||||
} else {
|
|
||||||
this.#invalidDependenciesInverse.set(dependency, new Set([node]));
|
if (!inverseInvalidSet) {
|
||||||
|
inverseInvalidSet = new Set();
|
||||||
|
this.#invalidDependenciesInverse.set(dependency, inverseInvalidSet);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
inverseInvalidSet.add(node);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -163,10 +170,9 @@ export default class DependencyMap {
|
||||||
* @param nodes Record of nodes to sort
|
* @param nodes Record of nodes to sort
|
||||||
* @returns Array of node keys sorted by depth
|
* @returns Array of node keys sorted by depth
|
||||||
*/
|
*/
|
||||||
private sortNodesByDepth = (nodes: Record<string, true>): string[] => {
|
private sortNodesByDepth = (nodes: string[]): string[] => {
|
||||||
// Pre-compute depths for all nodes
|
// Pre-compute depths for all nodes
|
||||||
const nodeKeys = Object.keys(nodes);
|
const nodeDepths = nodes.map((node) => ({
|
||||||
const nodeDepths = nodeKeys.map((node) => ({
|
|
||||||
node,
|
node,
|
||||||
depth: node.split(".").length,
|
depth: node.split(".").length,
|
||||||
}));
|
}));
|
||||||
|
|
@ -186,15 +192,16 @@ export default class DependencyMap {
|
||||||
*/
|
*/
|
||||||
|
|
||||||
addNodes = (nodes: Record<string, true>, strict = true) => {
|
addNodes = (nodes: Record<string, true>, strict = true) => {
|
||||||
|
const newUnaddedNodes = Object.keys(nodes).filter(
|
||||||
|
(node) => !this.#nodes.has(node),
|
||||||
|
);
|
||||||
const nodesToAdd = strict
|
const nodesToAdd = strict
|
||||||
? Object.keys(nodes)
|
? newUnaddedNodes
|
||||||
: this.sortNodesByDepth(nodes);
|
: this.sortNodesByDepth(newUnaddedNodes);
|
||||||
|
|
||||||
let didUpdateGraph = false;
|
let didUpdateGraph = false;
|
||||||
|
|
||||||
for (const newNode of nodesToAdd) {
|
for (const newNode of nodesToAdd) {
|
||||||
if (this.#nodes.has(newNode)) continue;
|
|
||||||
|
|
||||||
// New node introduced to the graph.
|
// New node introduced to the graph.
|
||||||
this.#nodes.set(newNode, true);
|
this.#nodes.set(newNode, true);
|
||||||
// Check the paths that consumed this node before it was introduced.
|
// Check the paths that consumed this node before it was introduced.
|
||||||
|
|
@ -228,11 +235,15 @@ export default class DependencyMap {
|
||||||
this.#invalidDependencies.get(iNode)?.delete(newNode);
|
this.#invalidDependencies.get(iNode)?.delete(newNode);
|
||||||
didUpdateGraph = true;
|
didUpdateGraph = true;
|
||||||
|
|
||||||
if (this.#dependenciesInverse.has(newNode)) {
|
// Get or create the inverse dependencies set for the new node
|
||||||
this.#dependenciesInverse.get(newNode)?.add(iNode);
|
let inverseSet = this.#dependenciesInverse.get(newNode);
|
||||||
} else {
|
|
||||||
this.#dependenciesInverse.set(newNode, new Set([iNode]));
|
if (!inverseSet) {
|
||||||
|
inverseSet = new Set();
|
||||||
|
this.#dependenciesInverse.set(newNode, inverseSet);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
inverseSet.add(iNode);
|
||||||
}
|
}
|
||||||
|
|
||||||
this.#invalidDependenciesInverse.delete(newNode);
|
this.#invalidDependenciesInverse.delete(newNode);
|
||||||
|
|
@ -261,11 +272,14 @@ export default class DependencyMap {
|
||||||
|
|
||||||
if (!this.#nodes.has(iNode)) continue;
|
if (!this.#nodes.has(iNode)) continue;
|
||||||
|
|
||||||
if (this.#invalidDependenciesInverse.has(node)) {
|
let inverseInvalidSet = this.#invalidDependenciesInverse.get(node);
|
||||||
this.#invalidDependenciesInverse.get(node)?.add(iNode);
|
|
||||||
} else {
|
if (!inverseInvalidSet) {
|
||||||
this.#invalidDependenciesInverse.set(node, new Set([iNode]));
|
inverseInvalidSet = new Set();
|
||||||
|
this.#invalidDependenciesInverse.set(node, inverseInvalidSet);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
inverseInvalidSet.add(iNode);
|
||||||
}
|
}
|
||||||
|
|
||||||
this.#dependenciesInverse.delete(node);
|
this.#dependenciesInverse.delete(node);
|
||||||
|
|
|
||||||
|
|
@ -299,6 +299,46 @@ export const unsafeFunctionForEval = [
|
||||||
"Navigator",
|
"Navigator",
|
||||||
];
|
];
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Checks if a child property path starts with the parent property path
|
||||||
|
* using either dot notation (.) or bracket notation ([)
|
||||||
|
*
|
||||||
|
* @param parentPropertyPath - The parent property path
|
||||||
|
* @param childPropertyPath - The child property path to check
|
||||||
|
* @returns true if childPropertyPath is a child of parentPropertyPath
|
||||||
|
*
|
||||||
|
* @example
|
||||||
|
* isChildPropertyPathStartsWithParent("Table1", "Table1.data") // true
|
||||||
|
* isChildPropertyPathStartsWithParent("Table1", "Table1[0]") // true
|
||||||
|
* isChildPropertyPathStartsWithParent("Table1", "Table2.data") // false
|
||||||
|
*/
|
||||||
|
export const isChildPropertyPathStartsWithParent = (
|
||||||
|
parentPropertyPath: string,
|
||||||
|
childPropertyPath: string,
|
||||||
|
): boolean => {
|
||||||
|
if (!parentPropertyPath || !childPropertyPath) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
const parentLength = parentPropertyPath.length;
|
||||||
|
|
||||||
|
if (childPropertyPath.length <= parentLength) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Most common case: dot notation
|
||||||
|
if (childPropertyPath[parentLength] === ".") {
|
||||||
|
return childPropertyPath.startsWith(parentPropertyPath);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Less common case: bracket notation
|
||||||
|
if (childPropertyPath[parentLength] === "[") {
|
||||||
|
return childPropertyPath.startsWith(parentPropertyPath);
|
||||||
|
}
|
||||||
|
|
||||||
|
return false;
|
||||||
|
};
|
||||||
|
|
||||||
export const isChildPropertyPath = (
|
export const isChildPropertyPath = (
|
||||||
parentPropertyPath: string,
|
parentPropertyPath: string,
|
||||||
childPropertyPath: string,
|
childPropertyPath: string,
|
||||||
|
|
@ -308,8 +348,7 @@ export const isChildPropertyPath = (
|
||||||
): boolean => {
|
): boolean => {
|
||||||
return (
|
return (
|
||||||
(!strict && parentPropertyPath === childPropertyPath) ||
|
(!strict && parentPropertyPath === childPropertyPath) ||
|
||||||
childPropertyPath.startsWith(`${parentPropertyPath}.`) ||
|
isChildPropertyPathStartsWithParent(parentPropertyPath, childPropertyPath)
|
||||||
childPropertyPath.startsWith(`${parentPropertyPath}[`)
|
|
||||||
);
|
);
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1958,6 +1958,8 @@ export default class DataTreeEvaluator {
|
||||||
const changePaths: Set<string> = new Set(dependenciesOfRemovedPaths);
|
const changePaths: Set<string> = new Set(dependenciesOfRemovedPaths);
|
||||||
const configTree = this.getConfigTree();
|
const configTree = this.getConfigTree();
|
||||||
|
|
||||||
|
const updatedValuePathsLatencyStart = performance.now();
|
||||||
|
|
||||||
for (const pathArray of updatedValuePaths) {
|
for (const pathArray of updatedValuePaths) {
|
||||||
const fullPropertyPath = convertPathToString(pathArray);
|
const fullPropertyPath = convertPathToString(pathArray);
|
||||||
|
|
||||||
|
|
@ -2005,27 +2007,45 @@ export default class DataTreeEvaluator {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const updatedValuePathsLatency =
|
||||||
|
performance.now() - updatedValuePathsLatencyStart;
|
||||||
|
|
||||||
// If a nested property path has changed and someone (say x) is dependent on the parent of the said property,
|
// If a nested property path has changed and someone (say x) is dependent on the parent of the said property,
|
||||||
// x must also be evaluated. For example, the following relationship exists in dependency map:
|
// x must also be evaluated. For example, the following relationship exists in dependency map:
|
||||||
// < "Input1.defaultText" : ["Table1.selectedRow.email"] >
|
// < "Input1.defaultText" : ["Table1.selectedRow.email"] >
|
||||||
// If Table1.selectedRow has changed, then Input1.defaultText must also be evaluated because Table1.selectedRow.email
|
// If Table1.selectedRow has changed, then Input1.defaultText must also be evaluated because Table1.selectedRow.email
|
||||||
// is a nested property of Table1.selectedRow
|
// is a nested property of Table1.selectedRow
|
||||||
|
const addDependantsOfNestedPropertyPathsLatencyStart = performance.now();
|
||||||
const changePathsWithNestedDependants = addDependantsOfNestedPropertyPaths(
|
const changePathsWithNestedDependants = addDependantsOfNestedPropertyPaths(
|
||||||
Array.from(changePaths),
|
Array.from(changePaths),
|
||||||
this.inverseDependencies,
|
this.inverseDependencies,
|
||||||
);
|
);
|
||||||
|
const addDependantsOfNestedPropertyPathsLatency =
|
||||||
|
performance.now() - addDependantsOfNestedPropertyPathsLatencyStart;
|
||||||
|
const trimDependantChangePathsLatencyStart = performance.now();
|
||||||
const trimmedChangedPaths = trimDependantChangePaths(
|
const trimmedChangedPaths = trimDependantChangePaths(
|
||||||
changePathsWithNestedDependants,
|
changePathsWithNestedDependants,
|
||||||
this.dependencies,
|
this.dependencies,
|
||||||
);
|
);
|
||||||
|
const trimDependantChangePathsLatency =
|
||||||
|
performance.now() - trimDependantChangePathsLatencyStart;
|
||||||
|
|
||||||
// Now that we have all the root nodes which have to be evaluated, recursively find all the other paths which
|
// Now that we have all the root nodes which have to be evaluated, recursively find all the other paths which
|
||||||
// would get impacted because they are dependent on the said root nodes and add them in order
|
// would get impacted because they are dependent on the said root nodes and add them in order
|
||||||
|
const completeSortOrderLatencyStart = performance.now();
|
||||||
const completeSortOrder = this.getCompleteSortOrder(
|
const completeSortOrder = this.getCompleteSortOrder(
|
||||||
trimmedChangedPaths,
|
trimmedChangedPaths,
|
||||||
this.inverseDependencies,
|
this.inverseDependencies,
|
||||||
);
|
);
|
||||||
|
const completeSortOrderLatency =
|
||||||
|
performance.now() - completeSortOrderLatencyStart;
|
||||||
|
|
||||||
|
this.logs.push({
|
||||||
|
updatedValuePathsLatency,
|
||||||
|
addDependantsOfNestedPropertyPathsLatency,
|
||||||
|
trimDependantChangePathsLatency,
|
||||||
|
completeSortOrderLatency,
|
||||||
|
});
|
||||||
|
|
||||||
// Remove any paths that do not exist in the data tree anymore
|
// Remove any paths that do not exist in the data tree anymore
|
||||||
return difference(
|
return difference(
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue
Block a user