Poor path performance of JsonPathMatcher for Yaml ChangeKey and ChangeValue on large files

[jrgleason]

What problem are you trying to solve?

I have a project with a bunch of YAML files. When I try to add a recipe that runs ChangeKey against a complex oldKeyPath it causes huge lags (in my case a dry run goes from about 6 mins to over 1 hour). To get around this I started creating a simple precondition that scans the whole file for a string (typically the key or another key that is more unique). I understand this has some pitfalls and also doesn't help is a 3rd party recipe uses it and doesn't upgrade so I was hoping we could come up with a better solution.

Describe the solution you'd like

Maybe a parameter to the recipe that allows us to pass a search string as well to run the regex before the matcher. It should be done before the matcher is run here...

rewrite/rewrite-yaml/src/main/java/org/openrewrite/yaml/ChangeKey.java

Line 61 in 420f56d

if (matcher.matches(getCursor())) {

Have you considered any alternatives or workarounds?

My work around, as I said, is to use a precondition to check the file for some text before running the matcher

Additional context

I found this issue through my company and I am working on trying to get approved to contribute. When this happens I should be able to provide an example. Till then I will try to recreate one on my own but in theory this should be true of any project with a lot of yaml files (maybe even just a lot of files).

Are you interested in contributing this feature to OpenRewrite?

Sure but need to finish clearance first

[jrgleason]

Problem is also true of ChangeValue

[jrgleason]

Ok more info, it wasnt the amount of files but rather 1 file at 1.6mb. I would still recommend the optional regex param as a way to mitigate it as it tears through the file way faster.

[timtebeek]

Thanks for the added details here @jrgleason ! Let us know if you're indeed cleared to contribute. Ideally we improve performance of that JsonPathMatcher.matches without any additional arguments, but I'm not sure if that has been profiled and tried before:

rewrite/rewrite-yaml/src/main/java/org/openrewrite/yaml/JsonPathMatcher.java

Lines 84 to 103 in a126fc8

	public boolean matches(Cursor cursor) {
	Object cursorValue = new ReplaceAliasWithAnchorValueVisitor<Integer>().visit((Tree)cursor.getValue(), 0);
	List<Object> cursorPath = cursor.getPathAsStream()
	.map(cp -> {
	if (cp instanceof Yaml) {
	cp = new ReplaceAliasWithAnchorValueVisitor<Integer>().visit((Yaml) cp, 0);
	}
	return cp;
	})
	.collect(Collectors.toList());
	return find(cursor).map(o -> {
	if (o instanceof List) {
	//noinspection unchecked
	List<Object> l = (List<Object>) o;
	return !disjoint(l, cursorPath) && l.contains(cursorValue);
	} else {
	return Objects.equals(o, cursorValue);
	}
	}).orElse(false);
	}

[timtebeek]

For context: did your large yaml file perhaps contain anchors? I see those are expanded, so they can also add to the complexity and run duration.

[knutwannheden]

The support to resolve references to YAML anchors was added way back in #2272. I think this will definitely cause performance issues (for multiple reasons). I am thinking we should maybe just remove support for YAML references for now, because I get the impression that YAML references aren't correctly implemented anyway. Alternatively, we could look into fixing the performance issues it is causing.

[knutwannheden]

@jrgleason I've improved the performance of the JsonPathMatcher in the following commits: 0f76203, f71dc0d, ae90839, 0f8e755, f1f48c0, and f30cd55. These changes should make the matching quite a bit faster. Are you able to check if this improves things for you? There is still more that can be done.