Reduce ratchet memory usage for multi-module projects

[laurentgo]

Ratchet creates a cache of Git repository per project but each Git repository is a new FileRepository instance even if two projects share the same repository on disk.

Since FileRepository is thread-safe, change GitRatchet to cache FileRepository instances per git repository

Please DO NOT FORCE PUSH. Don't worry about messy history, it's easier to do code review if we can tell what happened after the review, and force pushing breaks that.

Please make sure that your PR allows edits from maintainers. Sometimes its faster for us to just fix something than it is to describe how to fix it.

After creating the PR, please add a commit that adds a bullet-point under the [Unreleased] section of CHANGES.md, plugin-gradle/CHANGES.md, and plugin-maven/CHANGES.md which includes:

• a summary of the change
• either
  • a link to the issue you are resolving (for small changes)
  • a link to the PR you just created (for big changes likely to have discussion)

If your change only affects a build plugin, and not the lib, then you only need to update the plugin-foo/CHANGES.md for that plugin.

If your change affects lib in an end-user-visible way (fixing a bug, updating a version) then you need to update CHANGES.md for both the lib and all build plugins. Users of a build plugin shouldn't have to refer to lib to see changes that affect them.

This makes it easier for the maintainers to quickly release your changes :)

[nedtwigg]

Did you profile this (System.out.println would work) to confirm that it's actually creating fewer Repository instances? They are supposed to be cached already thanks to the gitRoots map

spotless/lib-extra/src/main/java/com/diffplug/spotless/extra/GitRatchet.java

Lines 149 to 170 in 8d1467f

	protected Repository repositoryFor(Project project) throws IOException {
	Repository repo = gitRoots.get(project);
	if (repo == null) {
	if (isGitRoot(getDir(project))) {
	repo = createRepo(getDir(project));
	} else {
	Project parentProj = getParent(project);
	if (parentProj == null) {
	repo = traverseParentsUntil(getDir(project).getParentFile(), null);
	if (repo == null) {
	throw new IllegalArgumentException("Cannot find git repository in any parent directory");
	}
	} else {
	repo = traverseParentsUntil(getDir(project).getParentFile(), getDir(parentProj));
	if (repo == null) {
	repo = repositoryFor(parentProj);
	}
	}
	}
	gitRoots.put(project, repo);
	}
	return repo;

But maybe there's a mistake.

[laurentgo]

Did you profile this (System.out.println would work) to confirm that it's actually creating fewer Repository instances? They are supposed to be cached already thanks to the gitRoots map

spotless/lib-extra/src/main/java/com/diffplug/spotless/extra/GitRatchet.java

Lines 149 to 170 in 8d1467f

	protected Repository repositoryFor(Project project) throws IOException {
	Repository repo = gitRoots.get(project);
	if (repo == null) {
	if (isGitRoot(getDir(project))) {
	repo = createRepo(getDir(project));
	} else {
	Project parentProj = getParent(project);
	if (parentProj == null) {
	repo = traverseParentsUntil(getDir(project).getParentFile(), null);
	if (repo == null) {
	throw new IllegalArgumentException("Cannot find git repository in any parent directory");
	}
	} else {
	repo = traverseParentsUntil(getDir(project).getParentFile(), getDir(parentProj));
	if (repo == null) {
	repo = repositoryFor(parentProj);
	}
	}
	}
	gitRoots.put(project, repo);
	}
	return repo;

But maybe there's a mistake.

I actually profiled the whole build and inspected the heap before and after. Before my change, there was a different Repository instance per project and I guess it's because traverseParentsUntil doesn't use the cache at all?

Tbh I was not sure what was the full purpose of gitRoots (was the intent also to cache repositories directly in it?) but based on your comment, it seems it was, and I may have an idea on how to directly back this into gitRoot vs introducing a level of indirection.

[laurentgo]

After reading back my previous comment, I realized that something was amiss and so I re-did a debugging session.

The main issue is around the usage of isGitRoot: the name is actually misleading a bit and the method returns true if the provided File is located inside a git repository (vs returning true if the provided argument is the root of a git repository). This leads to repositoryFor method to always take the first branch of this test and never attempting to traverse the parents.

I still believe there's another issue with the second branch: if a project has no parent or if a child project is not located under its parent, then cache is not being used although those projects could share the same git root.

My change which uses git root location has the cache key actually addresses both issues.

[laurentgo]

@nedtwigg are you okay with the approach?

[nedtwigg]

So I think the old code was trying to say:

• finding a repository is a little bit expensive
• and it's cheap to determine the relationship between projects
• so let's take advantage of that so that we don't search for repositories more than we have to

But it had some subtle bugs, so it wasn't worth it. Thanks for cutting through the knot!

[nedtwigg]

Released in plugin-gradle 6.12.1 and plugin-maven 2.29.0.