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
Review URL: http://codereview.chromium.org/6883050

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

presubmit_canned_checks.py

@@ -67,11 +67,13 @@ 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.
+  errors = _FindNewViolationsOfRule(lambda line : keyword not in line,
-  for f, line_num, line in input_api.RightHandSideLines(lambda x: x):
+                                    input_api, file_filter)
-    if keyword in line:
+  text = '\n'.join('Found %s in %s' % (keyword, loc) for loc in errors)
-      text = 'Found ' + keyword + ' in %s, line %s' % (f.LocalPath(), line_num)
+  if text:
     return [output_api.PresubmitError(text)]
   return []
 
@@ -213,6 +215,41 @@ 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.
+    for line in f.NewContents():
+      if not callable_rule(line):
+        # Violation found in full text; re-run on just the changed contents.
+        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 +262,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):
+  tabs = _FindNewViolationsOfRule(lambda line : '\t' not in line,
-    if '\t' in line:
+                                  input_api, filter_more)
-      tabs.append('%s, line %s' % (f.LocalPath(), line_num))
+
   if tabs:
     return [output_api.PresubmitPromptWarning('Found a tab character in:',
                                               long_text='\n'.join(tabs))]
@@ -239,21 +276,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):
+  errors = _FindNewViolationsOfRule(lambda x : not unowned_todo.search(x),
-    if unowned_todo.search(line):
+                                    input_api, source_file_filter)
-      text = ('Found TO' + 'DO with no owner in %s, line %s' %
+  errors = ['Found TO' + 'DO with no owner in ' + x for x in errors]
-              (f.LocalPath(), line_num))
+  if errors:
-      return [output_api.PresubmitPromptWarning(text)]
+    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 = []
+  errors = _FindNewViolationsOfRule(lambda line : line.rstrip() == line,
-  for f, line_num, line in input_api.RightHandSideLines(source_file_filter):
+                                    input_api, source_file_filter)
-    if line.rstrip() != line:
-      errors.append('%s, line %s' % (f.LocalPath(), line_num))
   if errors:
     return [output_api.PresubmitPromptWarning(
         'Found line ending with white spaces in:',
@@ -265,28 +300,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 = []
+  def no_long_lines(line):
-  for f, line_num, line in input_api.RightHandSideLines(source_file_filter):
     # Allow lines with http://, https:// and #define/#pragma/#include/#if/#endif
     # to exceed the maxlen rule.
-    if (len(line) > maxlen and
+    return (len(line) <= maxlen or
-        not 'http://' in line and
+            any((url in line) for url in ('http://', 'https://')) or
-        not 'https://' in line and
+            line.startswith(('#define', '#include', '#import', '#pragma',
-        not line.startswith('#define') and
+                             '#if', '#endif')))
-        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
 
-  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 +910,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')
+    input_api1.AffectedFiles(mox.IgnoreArg(), include_deletes=False).AndReturn(
-    # Format is (file, line number, line content)
+        [affected_file])
-    output1 = [
+    affected_file.NewContents().AndReturn([
-      (affected_file, 42, 'yo, ' + content1),
+        'ahoy',
-      (affected_file, 43, 'yer'),
+        'yo' + content1,
-      (affected_file, 23, 'ya'),
+        'hay',
-    ]
+        'yer',
-    input_api1.RightHandSideLines(mox.IgnoreArg()).AndReturn(output1)
+        '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),
+    input_api2.AffectedFiles(mox.IgnoreArg(), include_deletes=False).AndReturn(
-      (affected_file, 43, 'yer'),
+        [affected_file])
-      (affected_file, 23, 'ya'),
+    affected_file.NewContents().AndReturn([
-    ]
+        'ahoy',
-    input_api2.RightHandSideLines(mox.IgnoreArg()).AndReturn(output2)
+        '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_files = (affected_file1, affected_file2,
-      (affected_file1, 42, 'yo, '),
+                      affected_file3, affected_file4)
-      (affected_file2, 43, 'yer\t'),
+
-      (affected_file3, 45, 'yr\t'),
+    def test(source_filter, include_deletes):
-      (affected_file4, 46, 'ye\t'),
+      self.assertFalse(include_deletes)
-    ]
+      for x in affected_files:
-    def test(source_filter):
+        if source_filter(x):
-      for i in output1:
+          yield x
-        if source_filter(i[0]):
-          yield i
     # 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,