presubmit checks: skip computing the diff if possible

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
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=82762

[ncarter (slow), 2011-04-19 19:28:12]

This speeds up my presubmits on a 4-line CL from ~7s to ~1s, as reported by the
script.  There are probably other ways to eliminate these ops, but this seemed
pretty straightforward.

ChangedContents() seems to cost 300ms-500ms per file, and was called once per
rule.  Kind of nasty.

[M-A Ruel, 2011-04-19 19:36:00]

I disagree, the right fix is to modify presubmit_support.py so:

- AffectedFile.NewContents() caches the FileRead() return value.
- *AffectedFile.GenerateScmDiff() caches it's return value.

It would be much simpler and would scale better with more presubmit checks.

[ncarter (slow), 2011-04-19 20:25:47]

On 2011/04/19 19:36:00, Marc-Antoine Ruel wrote:
> I disagree, the right fix is to modify presubmit_support.py so:
> 
> - AffectedFile.NewContents() caches the FileRead() return value.
> - *AffectedFile.GenerateScmDiff() caches it's return value.
> 
> It would be much simpler and would scale better with more presubmit checks.

I'm certainly open to other options -- I sent this out mainly to get a sense for
what the alternatives might be.

However, what you suggest would be slower, because we'd still call
GenerateScmDiff once per file.  This change makes it so that oftentimes,
GenerateScmDiff is never called.

[ncarter (slow), 2011-04-19 21:15:14]

On 2011/04/19 20:25:47, ncarter wrote:
> On 2011/04/19 19:36:00, Marc-Antoine Ruel wrote:
> > I disagree, the right fix is to modify presubmit_support.py so:
> > 
> > - AffectedFile.NewContents() caches the FileRead() return value.
> > - *AffectedFile.GenerateScmDiff() caches it's return value.
> > 
> > It would be much simpler and would scale better with more presubmit checks.
> 
> I'm certainly open to other options -- I sent this out mainly to get a sense
for
> what the alternatives might be.
> 
> However, what you suggest would be slower, because we'd still call
> GenerateScmDiff once per file.  This change makes it so that oftentimes,
> GenerateScmDiff is never called.

I implemented the caching you suggest (and removed my other optimization).

For a 4-file change where two of the files have whitespace errors:

No optimization: 9.2s
Caching NewContents: 9.1s
Caching ChangedContents: 2.7s
Skipping ChangedContents if NewContents is clean: 1.6s

[M-A Ruel, 2011-04-20 16:04:41]

Hey sorry for the delay!

LGTM for it, I haven't seen the cache patch uploaded and I'd like it to be
committed too.

Can you upload that as a separate change? Thanks.

[ncarter (slow), 2011-04-20 22:45:25]

Thanks for the LGTM.

Do you think the timing warnings (the snapshot() ones) are a good idea?  I was
assuming I'd throw them away before committing.

[M-A Ruel, 2011-04-20 23:27:19]

On 2011/04/20 22:45:25, ncarter wrote:
> Thanks for the LGTM.
> 
> Do you think the timing warnings (the snapshot() ones) are a good idea?  I was
> assuming I'd throw them away before committing.

Keep them, it's easy to remove if it's annoying.

[ncarter (slow), 2011-04-21 09:55:30]

I had to make changes to get the unit tests to pass.  PTAL.

[M-A Ruel, 2011-04-21 20:55:25]

lgtm

[Avi (use Gerrit), 2011-04-21 22:32:24]

This breaks git. With this change, moving a file in git is not noticed by the
presubmit and it dies.

http://pastebin.com/TAUsSXf2

Please revert.

[ncarter (slow), 2011-04-22 00:52:22]

Marc-Antoine: I've reproed & fixed the issue that Avi reported; please take
another look.

[M-A Ruel, 2011-04-22 01:25:38]

lgtm

[ncarter (slow), 2011-04-22 17:22:35]

Marc-Antoine: third time.  I think my personal share price is tanking this week
...

[M-A Ruel, 2011-04-23 00:37:28]

lgtm