threads / patch / 16218

patch, 2 partsCached the git configuration, which is now noticibly faster on windows.

Subject: [PATCH 2/2] Cached the git configuration, which is now noticibly faster on windows.

## tl;dr

11 messages between Nov 8, 2008 and Nov 12, 2008. Diffs are folded; open one to read it.

replies: 10people: 7as markdown or json

John Chapman· Nov 8, 2008, 03:22 UTC · lore

[PATCH 1/2] Added support for purged files and also optimised memory usage.

Purged files are handled as if they are merely deleted, which is not entirely optimal, but I don't know of any other way to handle them. File data is deleted from memory as early as they can, and they are more efficiently handled, at (significant) cost to CPU usage.

Still need to handle p4 branches with spaces in their names.
Still need to make git-p4 clone more reliable.
 - Perhaps with a --continue option. (Sometimes the p4 server kills
 the connection)
Signed-off-by: John Chapman <thestar@fussycoder.id.au>
---
 contrib/fast-import/git-p4 |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)
Show changes to contrib/fast-import/git-p4 +7 −7
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 2216cac..38d1a17 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -946,7 +946,7 @@ class P4Sync(Command):
 
             if includeFile:
                 filesForCommit.append(f)
-                if f['action'] != 'delete':
+                if f['action'] not in ('delete', 'purge'):
                     filesToRead.append(f)
 
         filedata = []
@@ -965,11 +965,11 @@ class P4Sync(Command):
         while j < len(filedata):
             stat = filedata[j]
             j += 1
-            text = [];
+            text = ''
             while j < len(filedata) and filedata[j]['code'] in ('text', 'unicode', 'binary'):
-                text.append(filedata[j]['data'])
+                text += filedata[j]['data']
+                del filedata[j]['data']
                 j += 1
-            text = ''.join(text)
 
             if not stat.has_key('depotFile'):
                 sys.stderr.write("p4 print fails with: %s\n" % repr(stat))
@@ -1038,7 +1038,7 @@ class P4Sync(Command):
                 continue
 
             relPath = self.stripRepoPath(file['path'], branchPrefixes)
-            if file["action"] == "delete":
+            if file["action"] in ("delete", "purge"):
                 self.gitStream.write("D %s\n" % relPath)
             else:
                 data = file['data']
@@ -1077,7 +1077,7 @@ class P4Sync(Command):
 
                 cleanedFiles = {}
                 for info in files:
-                    if info["action"] == "delete":
+                    if info["action"] in ("delete", "purge"):
                         continue
                     cleanedFiles[info["depotFile"]] = info["rev"]
 
@@ -1400,7 +1400,7 @@ class P4Sync(Command):
             if change > newestRevision:
                 newestRevision = change
 
-            if info["action"] == "delete":
+            if info["action"] in ("delete", "purge"):
                 # don't increase the file cnt, otherwise details["depotFile123"] will have gaps!
                 #fileCnt = fileCnt + 1
                 continue

-- 
1.6.0.3.643.g233db
John Chapman· Nov 8, 2008, 03:22 UTC · re: John Chapman · lore
Signed-off-by: John Chapman <thestar@fussycoder.id.au>
---
 contrib/fast-import/git-p4 |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Show changes to contrib/fast-import/git-p4 +4 −1
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 38d1a17..9f0a5f9 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -316,8 +316,11 @@ def gitBranchExists(branch):
                             stderr=subprocess.PIPE, stdout=subprocess.PIPE);
     return proc.wait() == 0;
 
+_gitConfig = {}
 def gitConfig(key):
-    return read_pipe("git config %s" % key, ignore_error=True).strip()
+    if not _gitConfig.has_key(key):
+        _gitConfig[key] = read_pipe("git config %s" % key, ignore_error=True).strip()
+    return _gitConfig[key]
 
 def p4BranchesInGit(branchesAreInRemotes = True):
     branches = {}

-- 
1.6.0.3.643.g233db
David Symonds· Nov 8, 2008, 05:19 UTC · re: John Chapman · lore

Re: [PATCH 2/2] Cached the git configuration, which is now noticibly faster on windows.

On Fri, Nov 7, 2008 at 7:22 PM, John Chapman <thestar@fussycoder.id.au> wrote:
Show 6 quoted lines
> +_gitConfig = {}
>  def gitConfig(key):
> -    return read_pipe("git config %s" % key, ignore_error=True).strip()
> +    if not _gitConfig.has_key(key):
> +        _gitConfig[key] = read_pipe("git config %s" % key, ignore_error=True).strip()
> +    return _gitConfig[key]

If this is truly a noticeable bottleneck on Windows, something like the following might be even better: (completely untested!)

_gitConfig = None
def gitConfig(key):
  if _gitConfig is None:
    lines = read_pipe("git config -l", ignore_error=True).readlines():
    _gitConfig = dict([l.strip().split('=', 1) for l in lines])
  return _gitConfig.get(key, None)
Dave.
Arafangion· Nov 8, 2008, 06:52 UTC · re: David Symonds · lore

Re: [PATCH 2/2] Cached the git configuration, which is now noticibly faster on windows.

On Fri, 2008-11-07 at 21:19 -0800, David Symonds wrote: <snip>

Show 6 quoted lines
> _gitConfig = None
> def gitConfig(key):
>   if _gitConfig is None:
>     lines = read_pipe("git config -l", ignore_error=True).readlines():
>     _gitConfig = dict([l.strip().split('=', 1) for l in lines])
>   return _gitConfig.get(key, None)

That certainly is better, if one can assume that git's configuration is small. (And relative to the memory usage of the script, it will definetly be small).

I shall give that a go, although the change won't make it even faster - I suspect that much of the performance penalty in windows is the pathetic fork() performance, particularly as the memory usage of the script increases. (If subprocess does fork() and exec() in order to open another process, in cygwin).

Thankyou.
Steve Frécinaux· Nov 10, 2008, 08:38 UTC · re: Arafangion · lore

Re: [PATCH 2/2] Cached the git configuration, which is now noticibly faster on windows.

Arafangion wrote:
Show 12 quoted lines
> On Fri, 2008-11-07 at 21:19 -0800, David Symonds wrote:
> <snip>
>> _gitConfig = None
>> def gitConfig(key):
>>   if _gitConfig is None:
>>     lines = read_pipe("git config -l", ignore_error=True).readlines():
>>     _gitConfig = dict([l.strip().split('=', 1) for l in lines])
>>   return _gitConfig.get(key, None)
> 
> That certainly is better, if one can assume that git's configuration is
> small. (And relative to the memory usage of the script, it will
> definetly be small).

What about using git config --get-regexp to only get the p4-related settings ?

I don't really know the options used by git-p4, but something like this seems like a good candidate to address every trade-off concerns?

git-config --get-regexp '^(p4|user)\.'
Jakub Narebski· Nov 8, 2008, 10:13 UTC · re: David Symonds · lore

Re: [PATCH 2/2] Cached the git configuration, which is now noticibly faster on windows.

"David Symonds" <dsymonds@gmail.com> writes:
Show 18 quoted lines
> On Fri, Nov 7, 2008 at 7:22 PM, John Chapman <thestar@fussycoder.id.au> wrote:
> 
> > +_gitConfig = {}
> >  def gitConfig(key):
> > -    return read_pipe("git config %s" % key, ignore_error=True).strip()
> > +    if not _gitConfig.has_key(key):
> > +        _gitConfig[key] = read_pipe("git config %s" % key, ignore_error=True).strip()
> > +    return _gitConfig[key]
> 
> If this is truly a noticeable bottleneck on Windows, something like
> the following might be even better:  (completely untested!)
> 
> _gitConfig = None
> def gitConfig(key):
>   if _gitConfig is None:
>     lines = read_pipe("git config -l", ignore_error=True).readlines():
>     _gitConfig = dict([l.strip().split('=', 1) for l in lines])
>   return _gitConfig.get(key, None)

Wouldn't it be better to use "git config -l -z", split lines at "\0" (NUL), and split key from value at first "\N" (CR)? This format was meant for scripts.

-- 
Jakub Narebski
Poland
ShadeHawk on #git
Junio C Hamano· Nov 9, 2008, 18:33 UTC · re: John Chapman · lore

Re: [PATCH 2/2] Cached the git configuration, which is now noticibly faster on windows.

These are patches to fast-import/git-p4, which you two seem to in charge of.

    From:	John Chapman <thestar@fussycoder.id.au>
    Subject: [PATCH 1/2] Added support for purged files and also optimised memory usage.
    Date:	Sat,  8 Nov 2008 14:22:48 +1100
    Message-Id: <1226114569-8506-1-git-send-email-thestar@fussycoder.id.au>
    From:	John Chapman <thestar@fussycoder.id.au>
    Subject: [PATCH 2/2] Cached the git configuration, which is now noticibly faster on windows.
    Date:	Sat,  8 Nov 2008 14:22:49 +1100
    Message-Id: <1226114569-8506-2-git-send-email-thestar@fussycoder.id.au>

It was unfortunately not immediately obvious from the Subject: line what these patches are about, and I am guessing you missed them because of that.

Han-Wen Nienhuys· Nov 10, 2008, 03:50 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] Cached the git configuration, which is now noticibly faster on windows.

Hi Junio,

I haven't been involved with git-p4 for a long time. I'm not really fit for judging these patches.

Junio C Hamano escreveu:
Show 15 quoted lines
> These are patches to fast-import/git-p4, which you two seem to in charge
> of.
> 
>     From:	John Chapman <thestar@fussycoder.id.au>
>     Subject: [PATCH 1/2] Added support for purged files and also optimised memory usage.
>     Date:	Sat,  8 Nov 2008 14:22:48 +1100
>     Message-Id: <1226114569-8506-1-git-send-email-thestar@fussycoder.id.au>
> 
>     From:	John Chapman <thestar@fussycoder.id.au>
>     Subject: [PATCH 2/2] Cached the git configuration, which is now noticibly faster on windows.
>     Date:	Sat,  8 Nov 2008 14:22:49 +1100
>     Message-Id: <1226114569-8506-2-git-send-email-thestar@fussycoder.id.au>
> 
> It was unfortunately not immediately obvious from the Subject: line what
> these patches are about, and I am guessing you missed them because of that.
-- 
 Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
Simon Hausmann· Nov 10, 2008, 09:46 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] Cached the git configuration, which is now noticibly faster on windows.

On Sunday 09 November 2008 Junio C Hamano, wrote:
Show 15 quoted lines
> These are patches to fast-import/git-p4, which you two seem to in charge
> of.
>
>     From:	John Chapman <thestar@fussycoder.id.au>
>     Subject: [PATCH 1/2] Added support for purged files and also optimised
> memory usage. Date:	Sat,  8 Nov 2008 14:22:48 +1100
>     Message-Id: <1226114569-8506-1-git-send-email-thestar@fussycoder.id.au>
>
>     From:	John Chapman <thestar@fussycoder.id.au>
>     Subject: [PATCH 2/2] Cached the git configuration, which is now
> noticibly faster on windows. Date:	Sat,  8 Nov 2008 14:22:49 +1100
>     Message-Id: <1226114569-8506-2-git-send-email-thestar@fussycoder.id.au>
>
> It was unfortunately not immediately obvious from the Subject: line what
> these patches are about, and I am guessing you missed them because of that.

Ack on both patches. The second one could be done better, as suggested in the follow-ups, but both are clearly an improvement :)

Simon
Junio C Hamano· Nov 12, 2008, 00:50 UTC · re: Simon Hausmann · lore

Re: [PATCH 2/2] Cached the git configuration, which is now noticibly faster on windows.

Simon Hausmann <simon@lst.de> writes:
Show 19 quoted lines
> On Sunday 09 November 2008 Junio C Hamano, wrote:
>> These are patches to fast-import/git-p4, which you two seem to in charge
>> of.
>>
>>     From:	John Chapman <thestar@fussycoder.id.au>
>>     Subject: [PATCH 1/2] Added support for purged files and also optimised
>> memory usage. Date:	Sat,  8 Nov 2008 14:22:48 +1100
>>     Message-Id: <1226114569-8506-1-git-send-email-thestar@fussycoder.id.au>
>>
>>     From:	John Chapman <thestar@fussycoder.id.au>
>>     Subject: [PATCH 2/2] Cached the git configuration, which is now
>> noticibly faster on windows. Date:	Sat,  8 Nov 2008 14:22:49 +1100
>>     Message-Id: <1226114569-8506-2-git-send-email-thestar@fussycoder.id.au>
>>
>> It was unfortunately not immediately obvious from the Subject: line what
>> these patches are about, and I am guessing you missed them because of that.
>
> Ack on both patches. The second one could be done better, as suggested
> in the follow-ups, but both are clearly an improvement :)
Thanks, both of you.  Will apply.
Arafangion· Nov 12, 2008, 11:54 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] Cached the git configuration, which is now noticibly faster on windows.

Thanks for making those patches more visible, however I do feel the need
to mention one thing I expected to have been raised during patch review:
1) The memory optimisation may cause significant slowdown, which wasn't
a big issue on my machine, perhaps because I'm _still_ trying to get it
to work on my particular repo. (It's still using too much memory), and I
have a very fast machine.  It switches git-p4 from a RAM-intensive app
to a somewhat-less-RAM-intensive app at the cost of also becomming much
more CPU-intensive.
On Tue, 2008-11-11 at 16:50 -0800, Junio C Hamano wrote:
Show 24 quoted lines
> Simon Hausmann <simon@lst.de> writes:
> 
> > On Sunday 09 November 2008 Junio C Hamano, wrote:
> >> These are patches to fast-import/git-p4, which you two seem to in charge
> >> of.
> >>
> >>     From:	John Chapman <thestar@fussycoder.id.au>
> >>     Subject: [PATCH 1/2] Added support for purged files and also optimised
> >> memory usage. Date:	Sat,  8 Nov 2008 14:22:48 +1100
> >>     Message-Id: <1226114569-8506-1-git-send-email-thestar@fussycoder.id.au>
> >>
> >>     From:	John Chapman <thestar@fussycoder.id.au>
> >>     Subject: [PATCH 2/2] Cached the git configuration, which is now
> >> noticibly faster on windows. Date:	Sat,  8 Nov 2008 14:22:49 +1100
> >>     Message-Id: <1226114569-8506-2-git-send-email-thestar@fussycoder.id.au>
> >>
> >> It was unfortunately not immediately obvious from the Subject: line what
> >> these patches are about, and I am guessing you missed them because of that.
> >
> > Ack on both patches. The second one could be done better, as suggested
> > in the follow-ups, but both are clearly an improvement :)
> 
> Thanks, both of you.  Will apply.
>

← back to recent threads