Memoize git shell commands

[adsr]

Description

We have a shell script that invokes chef install and chef export against a setup with ~30 cookbooks. It was annoyingly slow so we dug into why that was the case. We found that these commands call ChefCLI::CookbookProfiler::Git in a loop which ends up shelling out the same handful of git commands repeatedly. For example, one run produced the following duplicate git commands (sorted by dupe count):

 74   execve("/usr/bin/git", ["git", "rev-parse", "HEAD"], <snip>
 46   execve("/usr/bin/git", ["git", "rev-parse", "HEAD"], <snip>
 41   execve("/usr/bin/git", ["git", "rev-parse", "--abbrev-ref", "HEAD"], <snip>
 39   execve("/usr/bin/git", ["git", "diff-files", "--quiet"], <snip>
 25   execve("/usr/bin/git", ["git", "branch", "-r", "--contains", "<snip>"], <snip>
 23   execve("/usr/lib/git-core/git", ["/usr/lib/git-core/git", "status", "--porcelain=2", "-uno"], <snip>
 23   execve("/usr/bin/git", ["git", "config", "--get", "branch.HEAD.remote"], <snip>
 21   execve("/usr/bin/git", ["git", "diff-files", "--quiet"], <snip>
 19   execve("/usr/bin/git", ["git", "rev-parse", "--abbrev-ref", "HEAD"], <snip>
 17   execve("/usr/bin/git", ["git", "config", "--get", "branch.<snip>.remote"], <snip>
 16   execve("/usr/bin/git", ["git", "config", "--get", "remote.origin.url"], <snip>
 14   execve("/usr/bin/git", ["git", "branch", "-r", "--contains", "<snip>"], <snip>
 14   execve("/usr/bin/git", ["git", "branch", "-r", "--contains", "<snip>"], <snip>
 12   execve("/usr/bin/git", ["git", "config", "--get", "remote.origin.url"], <snip>
 11   execve("/usr/bin/git", ["git", "config", "--get", "branch.<snip>.remote"], <snip>
  9   execve("/usr/bin/git", ["git", "config", "--get", "branch.HEAD.remote"], <snip>
  7   execve("/usr/bin/git", ["git", "branch", "-r", "--contains", "<snip>"], <snip>
  5   execve("/usr/lib/git-core/git", ["/usr/lib/git-core/git", "status", "--porcelain=2", "-uno"], <snip>

These all seem to be cacheable at the repo-level. Adding a simple memoization brought down our run from ~45s to ~1s.

To be safer, we could wrap this is a git_memo method to make the memoization behavior more explicit. I thought I'd collect some initial feedback on the general idea first. For example this would break things if each cookbook were its own repo. I'm not sure if it's that an expected / supported use-case.

I modified the code to memoize at the repo-level, so cookbook-per-repo should continue to work.

Related Issue

n/a

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

• I have read the CONTRIBUTING document.
• I have run the pre-merge tests locally and they pass.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.
• All commits have been signed-off for the Developer Certificate of Origin.

[Stromweld]

For example this would break things if each cookbook were its own repo. I'm not sure if it's that an expected / supported use-case.

That is actually recommended best practice and highly used use case to have each cookbook as it's own repo.

[adsr]

For example this would break things if each cookbook were its own repo. I'm not sure if it's that an expected / supported use-case.

That is actually recommended best practice and highly used use case to have each cookbook as it's own repo.

Understood. Any ideas for how we might reap this perf win without forking Chef?

[Stromweld]

Only thing that comes to mind is by adding to knife config file a property to define cookbook mono repo or multiple repo approach and then use that to make decision to run your optimization or run the old loop.

Another approach I just thought of is to use git initially and then check if all cookbooks live in subdirectory and then use your memorize approach and break out of loop otherwise move on through loop for other cookbooks.

Just Ideas, haven't fully looked at code for viability or complexity of this kind of change.

[Stromweld]

@adsr a question I have also is around how your storing all your cookbooks? Are you using central artifact store like artifactory, internal chef supermarket, or chef-server? I have found dep resolution is a lot faster when using one of these options as it's able to download a single index and depsolve against that vs fetching each cookbook from git and parsing files for dependency information.

[adsr]

Only thing that comes to mind is by adding to knife config file a property to define cookbook mono repo or multiple repo approach and then use that to make decision to run your optimization or run the old loop.

Another approach I just thought of is to use git initially and then check if all cookbooks live in subdirectory and then use your memorize approach and break out of loop otherwise move on through loop for other cookbooks.

Just Ideas, haven't fully looked at code for viability or complexity of this kind of change.

OK, I have a few ideas as well after looking at things more carefully. The simplest move to support repo-per-cookbook would be to include repo path as part of the memo key. The other idea I had was to parallelize the commands. That would be a bigger change.

@adsr a question I have also is around how your storing all your cookbooks? Are you using central artifact store like artifactory, internal chef supermarket, or chef-server? I have found dep resolution is a lot faster when using one of these options as it's able to download a single index and depsolve against that vs fetching each cookbook from git and parsing files for dependency information.

In dev context, which is what I'm concerned with here, the cookbook store is the file system. I profiled the code a bit more carefully and found the dep solving code (starting from PolicyfileCompiler::graph_solution) occupies a small sliver of wall time compared to git-related code.

Here are rbspy flamegraphs of chef install and chef export with git.rb parts highlighted:

Alright, I just pushed a modified solution that keys the memo cache by repo path (git rev-parse --show-toplevel) as I mentioned above. It's not as big of a win because we are still calling this rev-parse command un-memoized many times. But it still gives us a big boost (~45s -> 8s) and should work with repo-per-cookbook setups too.

[Stromweld]

nice work

[Stromweld]

@adsr looks like there are a couple of cookstyle fixes needed. See buildkite job.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[adsr]

@Stromweld Sorry, missed those. Fixed.

[tpowell-progress]

@vkarve-chef came across this PR and looks like someone from Chef Workstation needs to weigh in to merge.

[vkarve-chef]

sure @tpowell-progress, we'll take a look in a week or two. thanks @adsr for creating the PR!

[ericnorris]

Hey @vkarve-chef, did your team have a chance to look at this? This would make a huge difference in the iteration speed of our local development process.

[vkarve-chef]

@ericnorris @adsr we intend to get to this by around mid-September. Sorry, it's taking us so long!
cc - @clintoncwolfe