pre-upload: cache repeated git operations
We have a lot of places that run the same git command on the same git
commit to get the same data. Cache the results across runs to avoid
that overhead.
Running on my local chromite branch with 15 commits, this cuts the time
from to process from ~37s to ~13s.
BUG=None
TEST=unittests pass
Change-Id: Ic920f734af8523fe31db29bd07e6661c03321250
Reviewed-on: https://chromium-review.googlesource.com/1802577
Tested-by: Mike Frysinger <vapier@chromium.org>
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Legacy-Commit-Queue: Commit Bot <commit-bot@chromium.org>
Reviewed-by: Allen Webb <allenwebb@google.com>
diff --git a/pre-upload.py b/pre-upload.py
index f2938d5..27059b4 100755
--- a/pre-upload.py
+++ b/pre-upload.py
@@ -113,6 +113,21 @@
# General Helpers
+class Cache(object):
+ """General helper for caching git content."""
+
+ def __init__(self):
+ self._cache = {}
+
+ def get_subcache(self, scope):
+ return self._cache.setdefault(scope, {})
+
+ def clear(self):
+ self._cache.clear()
+
+CACHE = Cache()
+
+
Project = collections.namedtuple('Project', ['name', 'dir', 'remote'])
@@ -248,19 +263,32 @@
a full file, you should check that first. One way to detect is that the
content will not have any newlines.
"""
+ # Make sure people don't accidentally pass in full paths which will never
+ # work. You need to use relative=True with _get_affected_files.
+ if path.startswith('/'):
+ raise ValueError('_get_file_content must be called with relative paths: %s'
+ % (path,))
+
+ # {<commit>: {<path1>: <content>, <path2>: <content>}}
+ cache = CACHE.get_subcache('get_file_content')
+ if path in cache:
+ return cache[path]
+
if commit == PRE_SUBMIT:
- return _run_command(['git', 'diff', 'HEAD', path])
+ content = _run_command(['git', 'diff', 'HEAD', path])
else:
- # Make sure people don't accidentally pass in full paths which will never
- # work. You need to use relative=True with _get_affected_files.
- if path.startswith('/'):
- raise ValueError('_get_file_content must be called with relative paths: '
- + path)
- return _run_command(['git', 'show', '%s:%s' % (commit, path)])
+ content = _run_command(['git', 'show', '%s:%s' % (commit, path)])
+ cache[path] = content
+ return content
def _get_file_diff(path, commit):
"""Returns a list of (linenum, lines) tuples that the commit touched."""
+ # {<commit>: {<path1>: <content>, <path2>: <content>}}
+ cache = CACHE.get_subcache('get_file_diff')
+ if path in cache:
+ return cache[path]
+
if commit == PRE_SUBMIT:
command = ['git', 'diff', '-p', '--pretty=format:', '--no-ext-diff', 'HEAD',
path]
@@ -280,6 +308,7 @@
new_lines.append((line_num, _try_utf8_decode(line[1:])))
if not line.startswith('-'):
line_num += 1
+ cache[path] = new_lines
return new_lines
@@ -370,7 +399,11 @@
'--name-only', 'HEAD']).split()
path = os.getcwd()
- files = git.RawDiff(path, '%s^!' % commit)
+ # {<commit>: {<path1>: <content>, <path2>: <content>}}
+ cache = CACHE.get_subcache('get_affected_files')
+ if path not in cache:
+ cache[path] = git.RawDiff(path, '%s^!' % commit)
+ files = cache[path]
# Filter out symlinks.
if not include_symlinks:
@@ -410,7 +443,13 @@
"""Returns the full commit message of a commit."""
if commit == PRE_SUBMIT:
return ''
- return _run_command(['git', 'log', '--format=%s%n%n%b', commit + '^!'])
+
+ # {<commit>: <content>}
+ cache = CACHE.get_subcache('get_commit_desc')
+ if commit not in cache:
+ cache[commit] = _run_command(['git', 'log', '--format=%s%n%n%b',
+ commit + '^!'])
+ return cache[commit]
def _check_lines_in_diff(commit, files, check_callable, error_description):
@@ -1939,6 +1978,8 @@
commit_count = len(commit_list)
hook_count = len(hooks)
for i, commit in enumerate(commit_list):
+ CACHE.clear()
+
error_list = []
for h, hook in enumerate(hooks):
output = ('PRESUBMIT.cfg: [%i/%i]: %s: Running [%i/%i] %s' %
diff --git a/pre-upload_unittest.py b/pre-upload_unittest.py
index 88d665b..da27cba 100755
--- a/pre-upload_unittest.py
+++ b/pre-upload_unittest.py
@@ -38,7 +38,14 @@
return pre_upload.Project(project_name, None, None)
-class TryUTF8DecodeTest(cros_test_lib.TestCase):
+class PreUploadTestCase(cros_test_lib.MockTestCase):
+ """Common test case base."""
+
+ def setUp(self):
+ pre_upload.CACHE.clear()
+
+
+class TryUTF8DecodeTest(PreUploadTestCase):
"""Verify we sanely handle unicode content."""
def runTest(self):
@@ -51,7 +58,7 @@
self.assertEquals('\x80', pre_upload._try_utf8_decode('\x80'))
-class CheckNoLongLinesTest(cros_test_lib.MockTestCase):
+class CheckNoLongLinesTest(PreUploadTestCase):
"""Tests for _check_no_long_lines."""
def setUp(self):
@@ -130,8 +137,9 @@
ProjectNamed('PROJECT'), 'COMMIT', options=['--exclude_regex=foo']))
-class CheckTabbedIndentsTest(cros_test_lib.MockTestCase):
+class CheckTabbedIndentsTest(PreUploadTestCase):
"""Tests for _check_tabbed_indents."""
+
def setUp(self):
self.PatchObject(pre_upload,
'_get_affected_files',
@@ -167,7 +175,7 @@
failure.items)
-class CheckProjectPrefix(cros_test_lib.MockTempDirTestCase):
+class CheckProjectPrefix(PreUploadTestCase, cros_test_lib.TempDirTestCase):
"""Tests for _check_project_prefix."""
def setUp(self):
@@ -222,7 +230,7 @@
pre_upload._check_project_prefix(ProjectNamed('PROJECT'), 'COMMIT'))
-class CheckFilePathCharTypeTest(cros_test_lib.MockTestCase):
+class CheckFilePathCharTypeTest(PreUploadTestCase):
"""Tests for _check_filepath_chartype."""
def setUp(self):
@@ -262,7 +270,7 @@
failure.items)
-class CheckKernelConfig(cros_test_lib.MockTestCase):
+class CheckKernelConfig(PreUploadTestCase):
"""Tests for _kernel_configcheck."""
def setUp(self):
@@ -295,7 +303,7 @@
self.assertFalse(failure)
-class CheckJson(cros_test_lib.MockTestCase):
+class CheckJson(PreUploadTestCase):
"""Tests for _run_json_check."""
def setUp(self):
@@ -334,7 +342,7 @@
self.content_mock.assert_called_once_with('/data.json', 'COMMIT')
-class CheckManifests(cros_test_lib.MockTestCase):
+class CheckManifests(PreUploadTestCase):
"""Tests _check_manifests."""
def setUp(self):
@@ -407,7 +415,7 @@
self._testReject('EBUILD foo\n')
-class CheckConfigParsing(cros_test_lib.MockTestCase):
+class CheckConfigParsing(PreUploadTestCase):
"""Tests _check_cq_ini_well_formed."""
def setUp(self):
@@ -447,7 +455,7 @@
self.assertTrue(isinstance(m, pre_upload.HookFailure))
-class CheckPortageMakeUseVar(cros_test_lib.MockTestCase):
+class CheckPortageMakeUseVar(PreUploadTestCase):
"""Tests for _check_portage_make_use_var."""
def setUp(self):
@@ -495,7 +503,7 @@
self.assertFalse(failure, failure)
-class CheckEbuildEapi(cros_test_lib.MockTestCase):
+class CheckEbuildEapi(PreUploadTestCase):
"""Tests for _check_ebuild_eapi."""
PORTAGE_STABLE = ProjectNamed('chromiumos/overlays/portage-stable')
@@ -595,7 +603,7 @@
self.assertEqual(ret, None)
-class CheckEbuildKeywords(cros_test_lib.MockTestCase):
+class CheckEbuildKeywords(PreUploadTestCase):
"""Tests for _check_ebuild_keywords."""
def setUp(self):
@@ -664,7 +672,7 @@
self._CheckContent('# HEADER\nKEYWORDS="~arm x86"\nblah\n', True)
-class CheckEbuildVirtualPv(cros_test_lib.MockTestCase):
+class CheckEbuildVirtualPv(PreUploadTestCase):
"""Tests for _check_ebuild_virtual_pv."""
PORTAGE_STABLE = ProjectNamed('chromiumos/overlays/portage-stable')
@@ -745,7 +753,7 @@
ret = pre_upload._check_ebuild_virtual_pv(self.PRIVATE_VARIANT_OVERLAY, 'H')
self.assertEqual(ret, None)
-class CheckCrosLicenseCopyrightHeader(cros_test_lib.MockTestCase):
+class CheckCrosLicenseCopyrightHeader(PreUploadTestCase):
"""Tests for _check_cros_license."""
def setUp(self):
@@ -847,7 +855,7 @@
self.content_mock.return_value = ('owner@chromium.org')
self.assertFalse(pre_upload._check_cros_license('proj', 'sha1'))
-class CheckAOSPLicenseCopyrightHeader(cros_test_lib.MockTestCase):
+class CheckAOSPLicenseCopyrightHeader(PreUploadTestCase):
"""Tests for _check_aosp_license."""
def setUp(self):
@@ -930,7 +938,7 @@
self.content_mock.return_value = ('owner@chromium.org')
self.assertEqual(None, pre_upload._check_aosp_license('proj', 'sha1'))
-class CheckLayoutConfTestCase(cros_test_lib.MockTestCase):
+class CheckLayoutConfTestCase(PreUploadTestCase):
"""Tests for _check_layout_conf."""
def setUp(self):
@@ -1017,7 +1025,7 @@
self.assertRejected(['metadata/layout.conf'])
-class CommitMessageTestCase(cros_test_lib.MockTestCase):
+class CommitMessageTestCase(PreUploadTestCase):
"""Test case for funcs that check commit messages."""
def setUp(self):
@@ -1333,7 +1341,7 @@
src_file=src_file, dst_file=dst_file)
-class HelpersTest(cros_test_lib.MockTempDirTestCase):
+class HelpersTest(PreUploadTestCase, cros_test_lib.TempDirTestCase):
"""Various tests for utility functions."""
def setUp(self):
@@ -1433,7 +1441,7 @@
self.assertEquals(pre_upload._get_affected_files('HEAD', relative=True), [])
-class ExecFilesTest(cros_test_lib.MockTestCase):
+class ExecFilesTest(PreUploadTestCase):
"""Tests for _check_exec_files."""
def setUp(self):
@@ -1476,7 +1484,7 @@
self.assertEqual(pre_upload._check_exec_files('proj', 'commit'), None)
-class CheckForUprev(cros_test_lib.MockTempDirTestCase):
+class CheckForUprev(PreUploadTestCase, cros_test_lib.TempDirTestCase):
"""Tests for _check_for_uprev."""
def setUp(self):
@@ -1563,7 +1571,7 @@
self.assertAccepted([DiffEntry(src_file='c/p/files/f', status='M')])
-class DirectMainTest(cros_test_lib.MockTempDirTestCase):
+class DirectMainTest(PreUploadTestCase, cros_test_lib.TempDirTestCase):
"""Tests for direct_main()"""
def setUp(self):
@@ -1623,7 +1631,7 @@
mock.ANY, proj_dir=mock.ANY, commit_list=commits, presubmit=mock.ANY)
-class CheckRustfmtTest(cros_test_lib.MockTestCase):
+class CheckRustfmtTest(PreUploadTestCase):
"""Tests for _check_rustfmt."""
def setUp(self):
