Optimize presubmit checks for win32 where determining the diff

seems to take an inordinate amount of time.  The idea is to
lint whole files, and only try to compute deltas if the file
contains at least one error.

For my environment, the three affected presubmit rules took
2000ms each for a 4-file changelist.  With this change,
all my rules run in about a second, and none take anything
close to 500ms.

Also, since there seem to be environment-dependent factors at
work here, I'm proposing putting timer warnings that print
a message if any check takes longer than 500ms.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=82559
Reverted: http://src.chromium.org/viewvc/chrome?view=rev&revision=82568
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=82607
Reverted: http://src.chromium.org/viewvc/chrome?view=rev&revision=82653
Review URL: http://codereview.chromium.org/6883050

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@82762 0039d316-1c4b-4281-b951-d872f2087c98

presubmit_canned_checks.py

@@ -67,12 +67,14 @@ def CheckChangeHasDescription(input_api, output_api):
 
 def CheckDoNotSubmitInFiles(input_api, output_api):
   """Checks that the user didn't add 'DO NOT ' + 'SUBMIT' to any files."""
+  # We want to check every text file, not just source files.
+  file_filter = lambda x : x
   keyword = 'DO NOT ' + 'SUBMIT'
-  # We want to check every text files, not just source files.
-  for f, line_num, line in input_api.RightHandSideLines(lambda x: x):
-    if keyword in line:
-      text = 'Found ' + keyword + ' in %s, line %s' % (f.LocalPath(), line_num)
-      return [output_api.PresubmitError(text)]
+  errors = _FindNewViolationsOfRule(lambda line : keyword not in line,
+                                    input_api, file_filter)
+  text = '\n'.join('Found %s in %s' % (keyword, loc) for loc in errors)
+  if text:
+    return [output_api.PresubmitError(text)]
   return []
 
 
@@ -213,6 +215,42 @@ def CheckChangeHasNoCrAndHasOnlyOneEol(input_api, output_api,
   return outputs
 
 
+def _ReportErrorFileAndLine(filename, line_num, line):
+  """Default error formatter for _FindNewViolationsOfRule."""
+  return '%s, line %s' % (filename, line_num)
+
+
+def _FindNewViolationsOfRule(callable_rule, input_api, source_file_filter=None,
+                             error_formatter=_ReportErrorFileAndLine):
+  """Find all newly introduced violations of a per-line rule (a callable).
+
+  Arguments:
+    callable_rule: a callable taking a line of input and returning True
+      if the rule is satisfied and False if there was a problem.
+    input_api: object to enumerate the affected files.
+    source_file_filter: a filter to be passed to the input api.
+    error_formatter: a callable taking (filename, line_number, line) and
+      returning a formatted error string.
+
+  Returns:
+    A list of the newly-introduced violations reported by the rule.
+  """
+  errors = []
+  for f in input_api.AffectedFiles(source_file_filter, include_deletes=False):
+    # For speed, we do two passes, checking first the full file.  Shelling out
+    # to the SCM to determine the changed region can be quite expensive on
+    # Win32.  Assuming that most files will be kept problem-free, we can
+    # skip the SCM operations most of the time.
+    if all(callable_rule(line) for line in f.NewContents()):
+      continue  # No violation found in full text: can skip considering diff.
+
+    for line_num, line in f.ChangedContents():
+      if not callable_rule(line):
+        errors.append(error_formatter(f.LocalPath(), line_num, line))
+
+  return errors
+
+
 def CheckChangeHasNoTabs(input_api, output_api, source_file_filter=None):
   """Checks that there are no tab characters in any of the text files to be
   submitted.
@@ -225,10 +263,10 @@ def CheckChangeHasNoTabs(input_api, output_api, source_file_filter=None):
     return (not input_api.os_path.basename(affected_file.LocalPath()) in
                 ('Makefile', 'makefile') and
             source_file_filter(affected_file))
-  tabs = []
-  for f, line_num, line in input_api.RightHandSideLines(filter_more):
-    if '\t' in line:
-      tabs.append('%s, line %s' % (f.LocalPath(), line_num))
+
+  tabs = _FindNewViolationsOfRule(lambda line : '\t' not in line,
+                                  input_api, filter_more)
+
   if tabs:
     return [output_api.PresubmitPromptWarning('Found a tab character in:',
                                               long_text='\n'.join(tabs))]
@@ -239,21 +277,19 @@ def CheckChangeTodoHasOwner(input_api, output_api, source_file_filter=None):
   """Checks that the user didn't add TODO(name) without an owner."""
 
   unowned_todo = input_api.re.compile('TO' + 'DO[^(]')
-  for f, line_num, line in input_api.RightHandSideLines(source_file_filter):
-    if unowned_todo.search(line):
-      text = ('Found TO' + 'DO with no owner in %s, line %s' %
-              (f.LocalPath(), line_num))
-      return [output_api.PresubmitPromptWarning(text)]
+  errors = _FindNewViolationsOfRule(lambda x : not unowned_todo.search(x),
+                                    input_api, source_file_filter)
+  errors = ['Found TO' + 'DO with no owner in ' + x for x in errors]
+  if errors:
+    return [output_api.PresubmitPromptWarning('\n'.join(errors))]
   return []
 
 
 def CheckChangeHasNoStrayWhitespace(input_api, output_api,
                                     source_file_filter=None):
   """Checks that there is no stray whitespace at source lines end."""
-  errors = []
-  for f, line_num, line in input_api.RightHandSideLines(source_file_filter):
-    if line.rstrip() != line:
-      errors.append('%s, line %s' % (f.LocalPath(), line_num))
+  errors = _FindNewViolationsOfRule(lambda line : line.rstrip() == line,
+                                    input_api, source_file_filter)
   if errors:
     return [output_api.PresubmitPromptWarning(
         'Found line ending with white spaces in:',
@@ -265,28 +301,23 @@ def CheckLongLines(input_api, output_api, maxlen=80, source_file_filter=None):
   """Checks that there aren't any lines longer than maxlen characters in any of
   the text files to be submitted.
   """
-  bad = []
-  for f, line_num, line in input_api.RightHandSideLines(source_file_filter):
+  def no_long_lines(line):
     # Allow lines with http://, https:// and #define/#pragma/#include/#if/#endif
     # to exceed the maxlen rule.
-    if (len(line) > maxlen and
-        not 'http://' in line and
-        not 'https://' in line and
-        not line.startswith('#define') and
-        not line.startswith('#include') and
-        not line.startswith('#import') and
-        not line.startswith('#pragma') and
-        not line.startswith('#if') and
-        not line.startswith('#endif')):
-      bad.append(
-          '%s, line %s, %s chars' %
-          (f.LocalPath(), line_num, len(line)))
-      if len(bad) == 5:  # Just show the first 5 errors.
-        break
+    return (len(line) <= maxlen or
+            any((url in line) for url in ('http://', 'https://')) or
+            line.startswith(('#define', '#include', '#import', '#pragma',
+                             '#if', '#endif')))
 
-  if bad:
+  def format_error(filename, line_num, line):
+    return '%s, line %s, %s chars' % (filename, line_num, len(line))
+
+  errors = _FindNewViolationsOfRule(no_long_lines, input_api,
+                                    source_file_filter,
+                                    error_formatter=format_error)
+  if errors:
     msg = 'Found lines longer than %s characters (first 5 shown).' % maxlen
-    return [output_api.PresubmitPromptWarning(msg, items=bad)]
+    return [output_api.PresubmitPromptWarning(msg, items=errors[:5])]
   else:
     return []
 
@@ -880,24 +911,45 @@ def PanProjectChecks(input_api, output_api,
   text_files = lambda x: input_api.FilterSourceFile(x, black_list=black_list,
                                                     white_list=white_list)
 
+
+  snapshot_memory = []
+  def snapshot(msg):
+    """Measures & prints performance warning if a rule is running slow."""
+    dt2 = input_api.time.clock()
+    if snapshot_memory:
+      delta_ms = int(1000*(dt2 - snapshot_memory[0]))
+      if delta_ms > 500:
+        print "  %s took a long time: %dms" % (snapshot_memory[1], delta_ms)
+    snapshot_memory[:] = (dt2, msg)
+
   if owners_check:
+    snapshot("checking owners")
     results.extend(input_api.canned_checks.CheckOwners(
         input_api, output_api, source_file_filter=sources))
 
+  snapshot("checking long lines")
   results.extend(input_api.canned_checks.CheckLongLines(
       input_api, output_api, source_file_filter=sources))
+  snapshot( "checking tabs")
   results.extend(input_api.canned_checks.CheckChangeHasNoTabs(
       input_api, output_api, source_file_filter=sources))
+  snapshot( "checking stray whitespace")
   results.extend(input_api.canned_checks.CheckChangeHasNoStrayWhitespace(
       input_api, output_api, source_file_filter=sources))
+  snapshot("checking eol style")
   results.extend(input_api.canned_checks.CheckChangeSvnEolStyle(
       input_api, output_api, source_file_filter=text_files))
+  snapshot("checking svn mime types")
   results.extend(input_api.canned_checks.CheckSvnForCommonMimeTypes(
       input_api, output_api))
+  snapshot("checking license")
   results.extend(input_api.canned_checks.CheckLicense(
       input_api, output_api, license_header, source_file_filter=sources))
+  snapshot("checking nsobjects")
   results.extend(_CheckConstNSObject(
       input_api, output_api, source_file_filter=sources))
+  snapshot("checking singletons")
   results.extend(_CheckSingletonInHeaders(
       input_api, output_api, source_file_filter=sources))
+  snapshot("done")
   return results

tests/presubmit_unittest.py

@@ -1374,27 +1374,45 @@ class CannedChecksUnittest(PresubmitTestsBase):
     self.assertEquals(results2[0].__class__, error_type)
 
   def ContentTest(self, check, content1, content2, error_type):
+    """Runs a test of a content-checking rule.
+
+      Args:
+        check: the check to run.
+        content1: content which is expected to pass the check.
+        content2: content which is expected to fail the check.
+        error_type: the type of the error expected for content2.
+    """
     change1 = presubmit.Change(
         'foo1', 'foo1\n', self.fake_root_dir, None, 0, 0, None)
     input_api1 = self.MockInputApi(change1, False)
     affected_file = self.mox.CreateMock(presubmit.SvnAffectedFile)
-    affected_file.LocalPath().AndReturn('foo.cc')
-    # Format is (file, line number, line content)
-    output1 = [
-      (affected_file, 42, 'yo, ' + content1),
-      (affected_file, 43, 'yer'),
-      (affected_file, 23, 'ya'),
-    ]
-    input_api1.RightHandSideLines(mox.IgnoreArg()).AndReturn(output1)
+    input_api1.AffectedFiles(mox.IgnoreArg(), include_deletes=False).AndReturn(
+        [affected_file])
+    affected_file.NewContents().AndReturn([
+        'ahoy',
+        'yo' + content1,
+        'hay',
+        'yer',
+        'ya'])
+
     change2 = presubmit.Change(
         'foo2', 'foo2\n', self.fake_root_dir, None, 0, 0, None)
     input_api2 = self.MockInputApi(change2, False)
-    output2 = [
-      (affected_file, 42, 'yo, ' + content2),
-      (affected_file, 43, 'yer'),
-      (affected_file, 23, 'ya'),
-    ]
-    input_api2.RightHandSideLines(mox.IgnoreArg()).AndReturn(output2)
+
+    input_api2.AffectedFiles(mox.IgnoreArg(), include_deletes=False).AndReturn(
+        [affected_file])
+    affected_file.NewContents().AndReturn([
+        'ahoy',
+        'yo' + content2,
+        'hay',
+        'yer',
+        'ya'])
+    affected_file.ChangedContents().AndReturn([
+        (42, 'yo, ' + content2),
+        (43, 'yer'),
+        (23, 'ya')])
+    affected_file.LocalPath().AndReturn('foo.cc')
+
     self.mox.ReplayAll()
 
     results1 = check(input_api1, presubmit.OutputApi, None)
@@ -1565,20 +1583,21 @@ class CannedChecksUnittest(PresubmitTestsBase):
     # Only this one will trigger.
     affected_file4 = self.mox.CreateMock(presubmit.SvnAffectedFile)
     affected_file4.LocalPath().AndReturn('makefile.foo')
+    affected_file1.NewContents().AndReturn(['yo, '])
+    affected_file4.NewContents().AndReturn(['ye\t'])
+    affected_file4.ChangedContents().AndReturn([(46, 'ye\t')])
     affected_file4.LocalPath().AndReturn('makefile.foo')
-    output1 = [
-      (affected_file1, 42, 'yo, '),
-      (affected_file2, 43, 'yer\t'),
-      (affected_file3, 45, 'yr\t'),
-      (affected_file4, 46, 'ye\t'),
-    ]
-    def test(source_filter):
-      for i in output1:
-        if source_filter(i[0]):
-          yield i
+    affected_files = (affected_file1, affected_file2,
+                      affected_file3, affected_file4)
+
+    def test(source_filter, include_deletes):
+      self.assertFalse(include_deletes)
+      for x in affected_files:
+        if source_filter(x):
+          yield x
     # Override the mock of these functions.
     input_api1.FilterSourceFile = lambda x: x
-    input_api1.RightHandSideLines = test
+    input_api1.AffectedFiles = test
     self.mox.ReplayAll()
 
     results1 = presubmit_canned_checks.CheckChangeHasNoTabs(input_api1,