From e06cb4e55df8f78d151a538e7b711798466495ab Mon Sep 17 00:00:00 2001 From: "nick@chromium.org" Date: Sat, 23 Apr 2011 01:20:38 +0000 Subject: [PATCH] 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 | 124 +++++++++++++++++++++++++----------- tests/presubmit_unittest.py | 69 ++++++++++++-------- 2 files changed, 132 insertions(+), 61 deletions(-) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 7a8d98dca..d6b31ce13 100644 --- a/presubmit_canned_checks.py +++ b/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 diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 22c1496b2..a3bf7b957 100755 --- a/tests/presubmit_unittest.py +++ b/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,