[PATCHv1 0/3] git-p4: improvements to p4 "blocking"

[PATCHv1 0/3] git-p4: improvements to p4 "blocking"

[Luke Diamand]

This patchset improves the way that git-p4 sends requests in "blocks".

The problem comes about because the Perforce server limits the maximum
number of results it will send back (there are actually two different
limits).

This causes git-p4 failures if you ask for too much data.

In commit 96b2d54aee ("git-p4: use -m when running p4 changes",
2015-04-20), git-p4 learned to make requests in blocks, by default 512
in size.

The problem with this, however, is that it can be very slow on large
repositories - you might have millions of commits, although only a
handful actually relate to the code you are trying to clone. Each 512
block request takes a sizeable fraction of a second to complete.

There's a command-line option to increase the block size, but unless you
know about it, it won't be at all obvious that you could use this to
speed things up.

This change
~~~~~~~~~~~

The function p4CmdList() has been taught to raise an exception when it
gets an error, and in particular, to notice and report the two "too many
results" errors.

The code that does the blocking can now start out with a nice large
block size, and then reduce it if it sees an error.

Luke Diamand (3):
  git-p4: raise exceptions from p4CmdList based on error from p4 server
  git-p4: narrow the scope of exceptions caught when parsing an int
  git-p4: auto-size the block

 git-p4.py               | 72 +++++++++++++++++++++++++++++++++++------
 t/t9818-git-p4-block.sh | 11 +++++++
 2 files changed, 73 insertions(+), 10 deletions(-)

-- 
2.17.0.392.gdeb1a6e9b7

[PATCHv1 1/3] git-p4: raise exceptions from p4CmdList based on error from p4 server

[Luke Diamand]

This change lays some groundwork for better handling of rowcount errors
from the server, where it fails to send us results because we requested
too many.

It adds an option to p4CmdList() to return errors as a Python exception.

The exceptions are derived from P4Exception (something went wrong),
P4ServerException (the server sent us an error code) and
P4RequestSizeException (we requested too many rows/results from the
server database).

This makes makes the code that handles the errors a bit easier.

The default behavior is unchanged; the new code is enabled with a flag.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py | 44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 66652f8280..d856078ec8 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -566,10 +566,30 @@ def isModeExec(mode):
     # otherwise False.
     return mode[-3:] == "755"
 
+class P4Exception(Exception):
+    """ Base class for exceptions from the p4 client """
+    def __init__(self, exit_code):
+        self.p4ExitCode = exit_code
+
+class P4ServerException(Exception):
+    """ Base class for exceptions where we get some kind of marshalled up result from the server """
+    def __init__(self, exit_code, p4_result):
+        super(P4ServerException, self).__init__(exit_code)
+        self.p4_result = p4_result
+        self.code = p4_result[0]['code']
+        self.data = p4_result[0]['data']
+
+class P4RequestSizeException(P4ServerException):
+    """ One of the maxresults or maxscanrows errors """
+    def __init__(self, exit_code, p4_result, limit):
+        super(P4RequestSizeException, self).__init__(exit_code, p4_result)
+        self.limit = limit
+
 def isModeExecChanged(src_mode, dst_mode):
     return isModeExec(src_mode) != isModeExec(dst_mode)
 
-def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False):
+def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,
+        errors_as_exceptions=False):
 
     if isinstance(cmd,basestring):
         cmd = "-G " + cmd
@@ -616,9 +636,25 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False):
         pass
     exitCode = p4.wait()
     if exitCode != 0:
-        entry = {}
-        entry["p4ExitCode"] = exitCode
-        result.append(entry)
+        if errors_as_exceptions:
+            if len(result) > 0:
+                data = result[0].get('data')
+                if data:
+                    m = re.search('Too many rows scanned \(over (\d+)\)', data)
+                    if not m:
+                        m = re.search('Request too large \(over (\d+)\)', data)
+
+                    if m:
+                        limit = int(m.group(1))
+                        raise P4RequestSizeException(exitCode, result, limit)
+
+                raise P4ServerException(exitCode, result)
+            else:
+                raise P4Exception(exitCode)
+        else:
+            entry = {}
+            entry["p4ExitCode"] = exitCode
+            result.append(entry)
 
     return result
 
-- 
2.17.0.392.gdeb1a6e9b7

Re: [PATCHv1 1/3] git-p4: raise exceptions from p4CmdList based on error from p4 server

[Eric Sunshine]

On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand <luke@diamand.org> wrote:
> This change lays some groundwork for better handling of rowcount errors
> from the server, where it fails to send us results because we requested
> too many.
>
> It adds an option to p4CmdList() to return errors as a Python exception.
>
> The exceptions are derived from P4Exception (something went wrong),
> P4ServerException (the server sent us an error code) and
> P4RequestSizeException (we requested too many rows/results from the
> server database).
>
> This makes makes the code that handles the errors a bit easier.
>
> The default behavior is unchanged; the new code is enabled with a flag.
>
> Signed-off-by: Luke Diamand <luke@diamand.org>
> ---
> diff --git a/git-p4.py b/git-p4.py
> @@ -566,10 +566,30 @@ def isModeExec(mode):
> +class P4ServerException(Exception):
> +    """ Base class for exceptions where we get some kind of marshalled up result from the server """
> +    def __init__(self, exit_code, p4_result):
> +        super(P4ServerException, self).__init__(exit_code)
> +        self.p4_result = p4_result
> +        self.code = p4_result[0]['code']
> +        self.data = p4_result[0]['data']

The subsequent patches never seem to access any of these fields, so
it's difficult to judge whether it's worthwhile having 'code' and
'data' bits split out like this.

> @@ -616,9 +636,25 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False):
>      if exitCode != 0:
> -        entry = {}
> -        entry["p4ExitCode"] = exitCode
> -        result.append(entry)
> +        if errors_as_exceptions:
> +            if len(result) > 0:
> +                data = result[0].get('data')
> +                if data:
> +                    m = re.search('Too many rows scanned \(over (\d+)\)', data)
> +                    if not m:
> +                        m = re.search('Request too large \(over (\d+)\)', data)

Does 'p4' localize these error messages?

> +                    if m:
> +                        limit = int(m.group(1))
> +                        raise P4RequestSizeException(exitCode, result, limit)
> +
> +                raise P4ServerException(exitCode, result)
> +            else:
> +                raise P4Exception(exitCode)
> +        else:
> +            entry = {}
> +            entry["p4ExitCode"] = exitCode
> +            result.append(entry)

Re: [PATCHv1 1/3] git-p4: raise exceptions from p4CmdList based on error from p4 server

[Luke Diamand]

On 5 June 2018 at 10:54, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand <luke@diamand.org> wrote:
>> This change lays some groundwork for better handling of rowcount errors
>> from the server, where it fails to send us results because we requested
>> too many.
>>
>> It adds an option to p4CmdList() to return errors as a Python exception.
>>
>> The exceptions are derived from P4Exception (something went wrong),
>> P4ServerException (the server sent us an error code) and
>> P4RequestSizeException (we requested too many rows/results from the
>> server database).
>>
>> This makes makes the code that handles the errors a bit easier.
>>
>> The default behavior is unchanged; the new code is enabled with a flag.
>>
>> Signed-off-by: Luke Diamand <luke@diamand.org>
>> ---
>> diff --git a/git-p4.py b/git-p4.py
>> @@ -566,10 +566,30 @@ def isModeExec(mode):
>> +class P4ServerException(Exception):
>> +    """ Base class for exceptions where we get some kind of marshalled up result from the server """
>> +    def __init__(self, exit_code, p4_result):
>> +        super(P4ServerException, self).__init__(exit_code)
>> +        self.p4_result = p4_result
>> +        self.code = p4_result[0]['code']
>> +        self.data = p4_result[0]['data']
>
> The subsequent patches never seem to access any of these fields, so
> it's difficult to judge whether it's worthwhile having 'code' and
> 'data' bits split out like this.

These changes don't use it, but I thought that future changes might
need them, and perhaps when I put that code in I was thinking I might
need it myself.

>
>> @@ -616,9 +636,25 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False):
>>      if exitCode != 0:
>> -        entry = {}
>> -        entry["p4ExitCode"] = exitCode
>> -        result.append(entry)
>> +        if errors_as_exceptions:
>> +            if len(result) > 0:
>> +                data = result[0].get('data')
>> +                if data:
>> +                    m = re.search('Too many rows scanned \(over (\d+)\)', data)
>> +                    if not m:
>> +                        m = re.search('Request too large \(over (\d+)\)', data)
>
> Does 'p4' localize these error messages?

That's a good question.

The marshalled-up error from Perforce looks like this:

     ([{'generic': 35, 'code': 'error', 'data': "Too many rows scanned
(over 40); see 'p4 help maxscanrows'.\n", 'severity': 3}])

It turns out that Perforce open-sourced the P4 client in 2014 (I only
recently found this out) so we can actually look at the code now!

    https://swarm.workshop.perforce.com/projects/perforce_software-p4

Clone it like this:

mkdir p4 &&
(cd p4 && git init && git config --add git-p4.branchList p4/2018-1:2018-1) &&
P4USER=guest P4PORT=workshop.perforce.com:1666 git p4 clone
--detect-branches --destination p4  //guest/perforce_software/p4@all

Here's the code:

    // ErrorId graveyard: retired/deprecated ErrorIds.

    ErrorId MsgDb::MaxResults              = { ErrorOf( ES_DB, 32,
E_FAILED, EV_ADMIN, 1 ), "Request too large (over %maxResults%); see
'p4 help maxresults'." } ;//NOTRANS
    ErrorId MsgDb::MaxScanRows             = { ErrorOf( ES_DB, 61,
E_FAILED, EV_ADMIN, 1 ), "Too many rows scanned (over %maxScanRows%);
see 'p4 help maxscanrows'." } ;//NOTRANS


I don't think there's actually a way to make it return any language
other than English though. There's a P4LANGUAGE environment variable,
but it just says "this is for system integrators":

https://www.perforce.com/perforce/r15.2/manuals/cmdref/P4LANGUAGE.html

So I think probably the language is always English.

Luke

Re: [PATCHv1 1/3] git-p4: raise exceptions from p4CmdList based on error from p4 server

[Eric Sunshine]

On Tue, Jun 5, 2018 at 6:56 AM Luke Diamand <luke@diamand.org> wrote:
> On 5 June 2018 at 10:54, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand <luke@diamand.org> wrote:
> >> +                    m = re.search('Too many rows scanned \(over (\d+)\)', data)
> >> +                    if not m:
> >> +                        m = re.search('Request too large \(over (\d+)\)', data)
> >
> > Does 'p4' localize these error messages?
>
> That's a good question.
>
> It turns out that Perforce open-sourced the P4 client in 2014 (I only
> recently found this out) so we can actually look at the code now!
>
> Here's the code:
>
>     // ErrorId graveyard: retired/deprecated ErrorIds.

Hmm, the "too many rows" error you're seeing is retired/deprecated(?).

>     ErrorId MsgDb::MaxResults              = { ErrorOf( ES_DB, 32,
> E_FAILED, EV_ADMIN, 1 ), "Request too large (over %maxResults%); see
> 'p4 help maxresults'." } ;//NOTRANS
>     ErrorId MsgDb::MaxScanRows             = { ErrorOf( ES_DB, 61,
> E_FAILED, EV_ADMIN, 1 ), "Too many rows scanned (over %maxScanRows%);
> see 'p4 help maxscanrows'." } ;//NOTRANS
>
> I don't think there's actually a way to make it return any language
> other than English though. [...]
> So I think probably the language is always English.

The "NOTRANS" annotation on the error messages is reassuring.

Re: [PATCHv1 1/3] git-p4: raise exceptions from p4CmdList based on error from p4 server

[Luke Diamand]

On 5 June 2018 at 20:41, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Jun 5, 2018 at 6:56 AM Luke Diamand <luke@diamand.org> wrote:
>> On 5 June 2018 at 10:54, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> > On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand <luke@diamand.org> wrote:
>> >> +                    m = re.search('Too many rows scanned \(over (\d+)\)', data)
>> >> +                    if not m:
>> >> +                        m = re.search('Request too large \(over (\d+)\)', data)
>> >
>> > Does 'p4' localize these error messages?
>>
>> That's a good question.
>>
>> It turns out that Perforce open-sourced the P4 client in 2014 (I only
>> recently found this out) so we can actually look at the code now!
>>
>> Here's the code:
>>
>>     // ErrorId graveyard: retired/deprecated ErrorIds.
>
> Hmm, the "too many rows" error you're seeing is retired/deprecated(?).

There's some code elsewhere that suggests it's being kept alive:

    // Retired ErrorIds. We need to keep these so that clients
    // built with newer apis can commnunicate with older servers
    // still sending these.

    static ErrorId MaxResults; // DEPRECATED
    static ErrorId MaxScanRows; // DEPRECATED


>
>>     ErrorId MsgDb::MaxResults              = { ErrorOf( ES_DB, 32,
>> E_FAILED, EV_ADMIN, 1 ), "Request too large (over %maxResults%); see
>> 'p4 help maxresults'." } ;//NOTRANS
>>     ErrorId MsgDb::MaxScanRows             = { ErrorOf( ES_DB, 61,
>> E_FAILED, EV_ADMIN, 1 ), "Too many rows scanned (over %maxScanRows%);
>> see 'p4 help maxscanrows'." } ;//NOTRANS
>>
>> I don't think there's actually a way to make it return any language
>> other than English though. [...]
>> So I think probably the language is always English.
>
> The "NOTRANS" annotation on the error messages is reassuring.

I'll check it works OK on Windows; charset translation might cause a problem.

Re: [PATCHv1 1/3] git-p4: raise exceptions from p4CmdList based on error from p4 server

[Luke Diamand]

>>
>>>     ErrorId MsgDb::MaxResults              = { ErrorOf( ES_DB, 32,
>>> E_FAILED, EV_ADMIN, 1 ), "Request too large (over %maxResults%); see
>>> 'p4 help maxresults'." } ;//NOTRANS
>>>     ErrorId MsgDb::MaxScanRows             = { ErrorOf( ES_DB, 61,
>>> E_FAILED, EV_ADMIN, 1 ), "Too many rows scanned (over %maxScanRows%);
>>> see 'p4 help maxscanrows'." } ;//NOTRANS
>>>
>>> I don't think there's actually a way to make it return any language
>>> other than English though. [...]
>>> So I think probably the language is always English.
>>
>> The "NOTRANS" annotation on the error messages is reassuring.
>
> I'll check it works OK on Windows; charset translation might cause a problem.

Works OK on Windows with 2017.2 client/server.

[PATCHv1 2/3] git-p4: narrow the scope of exceptions caught when parsing an int

[Luke Diamand]

The current code traps all exceptions around some code which parses an
integer, and then talks to Perforce.

That can result in errors from Perforce being ignored. Change the code
to only catch the integer conversion exceptions.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index d856078ec8..b337562b39 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -959,7 +959,7 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):
         try:
             (changeStart, changeEnd) = p4ParseNumericChangeRange(parts)
             block_size = chooseBlockSize(requestedBlockSize)
-        except:
+        except ValueError:
             changeStart = parts[0][1:]
             changeEnd = parts[1]
             if requestedBlockSize:
-- 
2.17.0.392.gdeb1a6e9b7

[PATCHv1 3/3] git-p4: auto-size the block

[Luke Diamand]

git-p4 originally would fetch changes in one query. On large repos this
could fail because of the limits that Perforce imposes on the number of
items returned and the number of queries in the database.

To fix this, git-p4 learned to query changes in blocks of 512 changes,
However, this can be very slow - if you have a few million changes,
with each chunk taking about a second, it can be an hour or so.

Although it's possible to tune this value manually with the
"--changes-block-size" option, it's far from obvious to ordinary users
that this is what needs doing.

This change alters the block size dynamically by looking for the
specific error messages returned from the Perforce server, and reducing
the block size if the error is seen, either to the limit reported by the
server, or to half the current block size.

That means we can start out with a very large block size, and then let
it automatically drop down to a value that works without error, while
still failing correctly if some other error occurs.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py               | 26 +++++++++++++++++++++-----
 t/t9818-git-p4-block.sh | 11 +++++++++++
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index b337562b39..6736f81b60 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -48,7 +48,8 @@ def __str__(self):
 defaultLabelRegexp = r'[a-zA-Z0-9_\-.]+$'
 
 # Grab changes in blocks of this many revisions, unless otherwise requested
-defaultBlockSize = 512
+# The code should now automatically reduce the block size if it is required
+defaultBlockSize = 1<<20
 
 p4_access_checked = False
 
@@ -969,7 +970,8 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):
     changes = set()
 
     # Retrieve changes a block at a time, to prevent running
-    # into a MaxResults/MaxScanRows error from the server.
+    # into a MaxResults/MaxScanRows error from the server. If
+    # we _do_ hit one of those errors, turn down the block size
 
     while True:
         cmd = ['changes']
@@ -983,10 +985,24 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):
         for p in depotPaths:
             cmd += ["%s...@%s" % (p, revisionRange)]
 
+        # fetch the changes
+        try:
+            result = p4CmdList(cmd, errors_as_exceptions=True)
+        except P4RequestSizeException as e:
+            if not block_size:
+                block_size = e.limit
+            elif block_size > e.limit:
+                block_size = e.limit
+            else:
+                block_size = max(2, block_size // 2)
+
+            if verbose: print("block size error, retry with block size {0}".format(block_size))
+            continue
+        except P4Exception as e:
+            die('Error retrieving changes description ({0})'.format(e.p4ExitCode))
+
         # Insert changes in chronological order
-        for entry in reversed(p4CmdList(cmd)):
-            if entry.has_key('p4ExitCode'):
-                die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode']))
+        for entry in reversed(result):
             if not entry.has_key('change'):
                 continue
             changes.add(int(entry['change']))
diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
index 8840a183ac..e5ec9cdec8 100755
--- a/t/t9818-git-p4-block.sh
+++ b/t/t9818-git-p4-block.sh
@@ -129,6 +129,7 @@ test_expect_success 'Create a repo with multiple depot paths' '
 '
 
 test_expect_success 'Clone repo with multiple depot paths' '
+	test_when_finished cleanup_git &&
 	(
 		cd "$git" &&
 		git p4 clone --changes-block-size=4 //depot/pathA@all //depot/pathB@all \
@@ -138,6 +139,16 @@ test_expect_success 'Clone repo with multiple depot paths' '
 	)
 '
 
+test_expect_success 'Clone repo with self-sizing block size' '
+	test_when_finished cleanup_git &&
+	git p4 clone --changes-block-size=1000000 //depot@all --destination="$git" &&
+	(
+		cd "$git" &&
+		git log --oneline > log &&
+		test_line_count \> 10 log
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
2.17.0.392.gdeb1a6e9b7

Re: [PATCHv1 3/3] git-p4: auto-size the block

[Eric Sunshine]

On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand <luke@diamand.org> wrote:
> git-p4 originally would fetch changes in one query. On large repos this
> could fail because of the limits that Perforce imposes on the number of
> items returned and the number of queries in the database.
>
> To fix this, git-p4 learned to query changes in blocks of 512 changes,
> However, this can be very slow - if you have a few million changes,
> with each chunk taking about a second, it can be an hour or so.
>
> Although it's possible to tune this value manually with the
> "--changes-block-size" option, it's far from obvious to ordinary users
> that this is what needs doing.
>
> This change alters the block size dynamically by looking for the
> specific error messages returned from the Perforce server, and reducing
> the block size if the error is seen, either to the limit reported by the
> server, or to half the current block size.
>
> That means we can start out with a very large block size, and then let
> it automatically drop down to a value that works without error, while
> still failing correctly if some other error occurs.
>
> Signed-off-by: Luke Diamand <luke@diamand.org>
> ---
> diff --git a/git-p4.py b/git-p4.py
> @@ -48,7 +48,8 @@ def __str__(self):
>  # Grab changes in blocks of this many revisions, unless otherwise requested
> -defaultBlockSize = 512
> +# The code should now automatically reduce the block size if it is required
> +defaultBlockSize = 1<<20

As worded, the new comment only has value to someone who knew the old
behavior (before this patch), not for someone reading the code fresh.
However, if reworded, it might be meaningful to all readers (new and
old):

    # The block size is reduced automatically if required

> @@ -983,10 +985,24 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):
> +        try:
> +            result = p4CmdList(cmd, errors_as_exceptions=True)
> +        except P4RequestSizeException as e:
> +            if not block_size:
> +                block_size = e.limit
> +            elif block_size > e.limit:
> +                block_size = e.limit
> +            else:
> +                block_size = max(2, block_size // 2)
> +
> +            if verbose: print("block size error, retry with block size {0}".format(block_size))

Perhaps: s/retry/retrying/

> diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
> @@ -129,6 +129,7 @@ test_expect_success 'Create a repo with multiple depot paths' '
> +test_expect_success 'Clone repo with self-sizing block size' '
> +       test_when_finished cleanup_git &&
> +       git p4 clone --changes-block-size=1000000 //depot@all --destination="$git" &&
> +       (
> +               cd "$git" &&
> +               git log --oneline > log &&
> +               test_line_count \> 10 log
> +       )
> +'

Or, without a subshell (and dropping whitespace after redirection operator):

    git -C git log --oneline >log &&
    test_line_count \> 10 log

(not worth a re-roll)