Parallelize Perforce diff generation

Review Request #10554 — Created May 14, 2019 and updated

Information

RBTools
master
d87838e...

Reviewers

Parallelize Perforce diff generation

This change tweaks PerforceClient.diff to delegate diff generation to a
thread pool. This is motivated by the fact that Perforce diff generation
involves roundtrips to a Perforce server, and can be quite slow when
working with large changesets.

Resolves bug 4806.

  • nosetests rbtools/clients/tests/test_p4.py
  • Before this change, posting a review update for a change with 179 deleted
    files took over four minutes. After the change, my wait time is down to
    55s.
Description From Last Updated

Probably better to define this as its own function, rather than defining inline. Maybe generalize the name, call it _diff_file(). …

chipx86chipx86

You'll need double backticks to be valid ReStructuredText.

chipx86chipx86

Our diff generator won't handle the None: part right. You can just include the possibility of this in the description …

chipx86chipx86

Can you explicitly return None? We go for explicit rather than implicit here. Same applies below.

chipx86chipx86

Blank line in-between statements and blocks.

chipx86chipx86
Checks run (2 succeeded)
flake8 passed.
JSHint passed.
chipx86
  1. Hey Ryan! Sorry I haven't done a review on this until now.

    Very neat idea. It might be worth us exploring this for other types of repositories.

    One thing that stands out is that the order of files in the diff are probably not going to be preserved the way that it's currently generating them. Am I right about that? Would you be able to have this keep track of order?

  2. 
      
chipx86
  1. 
      
  2. rbtools/clients/perforce.py (Diff revision 1)
     
     
    Show all issues

    Probably better to define this as its own function, rather than defining inline. Maybe generalize the name, call it _diff_file().

    There's a performance hit to defining inline functions (likely won't matter much here, but it's bitten us before), and I could see doing something interesting down the road with having a standardized function that just diffs a single file, and have a standardized way to parallelize all the calls needed to those, simplifying what needs to be done in each implementation.

  3. rbtools/clients/perforce.py (Diff revision 1)
     
     
    Show all issues

    You'll need double backticks to be valid ReStructuredText.

  4. rbtools/clients/perforce.py (Diff revision 1)
     
     
     
     
     
     
    Show all issues

    Our diff generator won't handle the None: part right. You can just include the possibility of this in the description ("This may be None if there's an issue generating the diff. In that case, a warning will likely be emitted.").

  5. rbtools/clients/perforce.py (Diff revision 1)
     
     
    Show all issues

    Can you explicitly return None? We go for explicit rather than implicit here.

    Same applies below.

  6. rbtools/clients/perforce.py (Diff revision 1)
     
     
     
    Show all issues

    Blank line in-between statements and blocks.

  7.