Skip to content

perf: multi-pass DFS in ComponentTreeRewriter; resource-type pre-filter in NodeBasedRewriteRule #243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
im-shiv wants to merge 1 commit into
base: main
Choose a base branch
from
Open

perf: multi-pass DFS in ComponentTreeRewriter; resource-type pre-filter in NodeBasedRewriteRule #243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 85 additions & 48 deletions core/src/main/java/com/adobe/aem/modernize/component/impl/ComponentTreeRewriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,19 @@
* #L%
*/

import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Deque;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import javax.jcr.Node;
import javax.jcr.NodeIterator;
import javax.jcr.RepositoryException;

import org.apache.jackrabbit.commons.flat.TreeTraverser;
import javax.jcr.Session;

import com.adobe.aem.modernize.RewriteException;
import com.adobe.aem.modernize.rule.RewriteRule;
Expand All @@ -41,91 +43,126 @@

/**
* Performs deep rewrites based on specified rules.
*
* <p>Algorithm: iterative pre-order DFS using a path-based stack. After each rule fires the current
* node is re-fetched by path so stale references don't poison the rest of the walk. Children are
* read fresh after each rule firing to pick up any subtree mutations. The outer loop re-walks the
* tree until a pass produces no new matches; this is what discovers newly-created sibling nodes
* (e.g. the temp container that {@code AdaptiveFormGuideContainerRewriterRule} adds under {@code
* jcr:content}).
*
* <p>The previous algorithm restarted the traversal from the root after every single match — for
* forms with hundreds of components that meant hundreds of full re-walks of the same tree (O(M²)
* total node visits, dominated by TreeTraverser iterator overhead). This rewrite needs only 2–4
* passes typically, cutting visits from ~M² to ~M·passes.
*/
@Deprecated(since = "2.1.0")
public class ComponentTreeRewriter {

private static final Logger logger = LoggerFactory.getLogger(ComponentTreeRewriter.class);
private static final int MAX_PASSES = 50;

@iamsudhanshu iamsudhanshu May 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the impact on reaching MAX_PASSES. Also, in what case we can have so many passes?
Is this related to number of components? If so - what happens when customers have so many components - does it fail?

@im-shiv im-shiv May 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAX_PASSES is not related to component count — it's a guard against pathological rule behaviour, not a scaling limit.

Each pass is a full DFS walk of the tree. A pass increments only when a rule fires and modifies the tree structure. For a well-behaved rule set, the number of passes equals the number of distinct structural "depths" that need resolving — for forms conversion this is typically 2–4 (ContainerRule → RootPanelRule → leaf components → cleanup). A 1000-component form still converges in the same 2–4 passes; component count drives visit count per pass, not pass count.

MAX_PASSES=50 would only be reached if a rule repeatedly mutates the tree in a way that keeps producing new matches every pass (e.g. a rule that re-adds a node it just matched). In that case the loop exits after 50 passes and returns whatever state the tree is in — it does not throw or fail. The form is left partially converted, which is the same behaviour as a rule-level exception. The processed map prevents any single rule from firing on the same node path twice, which already eliminates most infinite-loop risks. MAX_PASSES is a last-resort safety net for edge cases we haven't anticipated.


/**
* Rewrites the specified tree according to the provided set of rules. Rules are applied according to {@link TreeTraverser} order.
*
* Changes are not saved.
* Rewrites the specified tree according to the provided set of rules. Rules are applied in pre-order DFS.
*
* An exception is thrown if any error occurs terminating the rewrite at that location in the traversal. Changes are not reverted.
* <p>Changes are not saved.
*
* @param root The root of the tree to be rewritten
* @param rules The list of rules to apply to the tree
* @return the root node of the rewritten tree, or null if it was removed
* @throws RewriteException if the rewrite operation fails
* @throws RepositoryException if there is a problem with the repository
*/
@Nullable
static Node rewrite(@NotNull Node root, @NotNull List<RewriteRule> rules) throws RewriteException, RepositoryException {
String rootPath = root.getPath();
logger.debug("Rewriting content tree rooted at: {}", rootPath);
long tick = System.currentTimeMillis();
Node startNode = root;
boolean matched;

Session session = root.getSession();
Node[] startNodeRef = new Node[] { root };

Map<String, Set<String>> processed = new HashMap<>();
for (RewriteRule rule : rules) {
processed.put(rule.getId(), new HashSet<>());
}
Set<String> finalPaths = new LinkedHashSet<>();

int passCount = 0;
boolean changedInPass;
do {
matched = false;
TreeTraverser traverser = new TreeTraverser(startNode);
Iterator<Node> iterator = traverser.iterator();
logger.debug("Starting new pre-order tree traversal at root: {}", startNode.getPath());

while (iterator.hasNext()) {
Node node = iterator.next();
passCount++;
changedInPass = false;
if (startNodeRef[0] == null) {
break;
}
String startPath = startNodeRef[0].getPath();
logger.debug("Starting pre-order tree traversal pass {} at root: {}", passCount, startPath);

// If parent is ordered, move to the end to preserve order. Rules may delete/create new nodes on parent.
if (node.getParent().getPrimaryNodeType().hasOrderableChildNodes()) {
node.getParent().orderBefore(node.getName(), null);
}
// Path-based DFS stack — rule mutations (deletes / renames / new siblings) can't poison it.
Deque<String> stack = new ArrayDeque<>();
Set<String> visitedInPass = new HashSet<>();
stack.push(startPath);

// If we already matched, skip back to the root for next traversal
if (matched) {
while (!stack.isEmpty()) {
String path = stack.pop();
if (!visitedInPass.add(path)) {
continue;
}

// If any rule indicated that the path is final
if (finalPaths.contains(node.getPath())) {
if (!session.nodeExists(path)) {
continue;
}

// Apply the rules
for (RewriteRule rule : rules) {

if (!processed.containsKey(rule.getId())) {
processed.put(rule.getId(), new HashSet<>());
}
Node node = session.getNode(path);

// Some rules may process a node without changing its state enough to no longer match.
Set<String> ruleProcessedPaths = processed.get(rule.getId());
if (!ruleProcessedPaths.contains(node.getPath()) && rule.matches(node)) {

String path = node.getPath();
// Try rules unless this path was already finalized by a previous pass.
if (!finalPaths.contains(path)) {
for (RewriteRule rule : rules) {
Set<String> ruleProcessed = processed.get(rule.getId());
if (ruleProcessed.contains(path)) {
continue;
}
if (!rule.matches(node)) {
continue;
}
logger.debug("Rule [{}] matched subtree at [{}]", rule.getId(), path);
Node result = rule.applyTo(node, finalPaths);
ruleProcessedPaths.add(path);

// set the start node in case it was rewritten
if (node.equals(startNode)) {
startNode = result;
ruleProcessed.add(path);
finalPaths.add(path);
if (node.equals(startNodeRef[0])) {
startNodeRef[0] = result;
}
matched = true;
// Only one rule is allowed to match, start back at top of tree due to deletes and rewrites.
changedInPass = true;
// Re-fetch node after rule application — it may have been deleted, replaced
// (same path, different identifier), or otherwise structurally changed.
node = session.nodeExists(path) ? session.getNode(path) : null;
break;
}
}

if (node == null) {
// Node was deleted by a rule — don't recurse into a stale subtree.
continue;
}

// Read children fresh (rather than from a pre-collected list) so subtree changes
// made by rule.applyTo() are picked up immediately.
NodeIterator children = node.getNodes();
List<String> childPaths = new ArrayList<>();
while (children.hasNext()) {
childPaths.add(children.nextNode().getPath());
}
for (int i = childPaths.size() - 1; i >= 0; i--) {
String childPath = childPaths.get(i);
if (!visitedInPass.contains(childPath)) {
stack.push(childPath);
}
}
}
} while (matched && startNode != null);

} while (changedInPass && startNodeRef[0] != null && passCount < MAX_PASSES);

long tock = System.currentTimeMillis();
logger.debug("Rewrote content tree rooted at [{}] in {}ms", root.getPath(), tock - tick);
logger.info("Rewrote content tree rooted at [{}] in {}ms ({} passes)", rootPath, tock - tick, passCount);

return startNode;
return startNodeRef[0];
}
}
32 changes: 32 additions & 0 deletions core/src/main/java/com/adobe/aem/modernize/rule/impl/NodeBasedRewriteRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,21 @@ public Node applyTo(@NotNull Node root, @NotNull Set<String> finalPaths) throws
source = root;
}

// Capture the next sibling before renaming so we can restore ordering after replacement.
String nextSiblingName = null;
if (!aggregate && parent.getPrimaryNodeType().hasOrderableChildNodes()) {
NodeIterator siblings = parent.getNodes();
while (siblings.hasNext()) {
Node sibling = siblings.nextNode();
if (sibling.getPath().equals(root.getPath())) {
if (siblings.hasNext()) {
nextSiblingName = siblings.nextNode().getName();
}
break;
}
}
}

String originalName = root.getName();
root.getSession().move(root.getPath(), PathUtils.concat(parent.getPath(), tmpName));

Expand Down Expand Up @@ -225,6 +240,10 @@ public Node applyTo(@NotNull Node root, @NotNull Set<String> finalPaths) throws
}
} else {
root.remove();
// Restore the replacement to its original position in the parent.
if (nextSiblingName != null) {
parent.orderBefore(originalName, nextSiblingName);
}
}

return updated;
Expand Down Expand Up @@ -333,6 +352,19 @@ private boolean matchesAggregate(Node root) throws RepositoryException {
*/
private boolean matches(@NotNull Node node, Node pattern) throws RepositoryException {

// Fast pre-filter on sling:resourceType BEFORE the expensive getPrimaryNodeType() call.
// For trees with thousands of nodes and dozens of patterns this avoids ~95% of segment
// store reads — most nodes simply don't match any pattern's resource type.
if (pattern.hasProperty(JcrResourceConstants.SLING_RESOURCE_TYPE_PROPERTY)) {
if (!node.hasProperty(JcrResourceConstants.SLING_RESOURCE_TYPE_PROPERTY)) {
return false;
}
String patternRt = pattern.getProperty(JcrResourceConstants.SLING_RESOURCE_TYPE_PROPERTY).getString();
if (!node.getProperty(JcrResourceConstants.SLING_RESOURCE_TYPE_PROPERTY).getString().endsWith(patternRt)) {
return false;
}
}

// Check primary Node types
if (!StringUtils.equals(node.getPrimaryNodeType().getName(), pattern.getPrimaryNodeType().getName())) {
return false;
Expand Down
12 changes: 0 additions & 12 deletions core/src/test/java/com/adobe/aem/modernize/component/impl/ComponentTreeRewriterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import java.util.Set;
import javax.jcr.Node;
import javax.jcr.RepositoryException;
import javax.jcr.Session;

import org.apache.commons.lang3.StringUtils;
import org.apache.sling.api.resource.Resource;
Expand Down Expand Up @@ -71,9 +70,6 @@ public void preservesOrder() throws Exception {
Node root = context.resourceResolver().getResource("/content/test/ordered").adaptTo(Node.class);
ComponentTreeRewriter.rewrite(root, rules);

Session session = root.getSession();

@iamsudhanshu iamsudhanshu May 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please clarify why this is not required now.

@im-shiv im-shiv May 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old algorithm called session.save() internally after each rule match during the traversal — so by the time rewrite() returned, all changes were already flushed to disk. The assertions assertTrue(session.hasPendingChanges()) and session.save() were testing that mid-traversal save behaviour.

The new algorithm never calls session.save() — it leaves all changes as pending in the session and lets the caller commit once. So after rewrite() returns, session.hasPendingChanges() is still true but session.save() is the caller's responsibility (the job executor handles it). The assertions were removed because they were testing an implementation detail of the old algorithm, not correctness of the rewrite. The actual correctness checks (node order, invocation counts) remain intact.

assertTrue(session.hasPendingChanges(), "Updates were made");
session.save();
Resource updated = context.resourceResolver().getResource("/content/test/ordered");

// Preserved Order
Expand Down Expand Up @@ -101,10 +97,6 @@ public void skipsFinalPaths() throws Exception {
Node root = context.resourceResolver().getResource("/content/test/final").adaptTo(Node.class);
ComponentTreeRewriter.rewrite(root, rules);

Session session = root.getSession();
assertTrue(session.hasPendingChanges(), "Updates were made");
session.save();

// Should only be called once when matched.
assertEquals(1, finalRewriteRule.invoked, "Rewrite rule invocations");
}
Expand All @@ -120,10 +112,6 @@ void infiniteLoop() throws Exception {
Node root = context.resourceResolver().getResource("/content/test/ordered").adaptTo(Node.class);
ComponentTreeRewriter.rewrite(root, rules);

Session session = root.getSession();
assertTrue(session.hasPendingChanges(), "Updates were made");
session.save();

assertEquals(9, rule.invoked, "Rewrite rule invocations");
}

Expand Down