[PATCH] Work around performance bug in subprocess.Popen.communicate()

[PATCH] Work around performance bug in subprocess.Popen.communicate()

[Karl Wiberg]

From: Karl Wiberg <kha@treskal.com>

In Python 2.4 (specifically, I tested with 2.4.6 on Ubuntu 9.04),
subprocess.Popen.communicate() seems to take time proportional to the
square of the size of the indata, which makes it ridiculously
expensive to write stack log entries when the diffs are large. Work
around the bug by calling subprocess.Popen.stdin.write() manually
instead of letting communicate() handle the indata.

The performance bug has been fixed in Python 2.6 (I tested with
2.6.2), so with that version this workaround doesn't affect the run
time. I haven't tested with Python 2.5.

This fixes bug 13319.

Signed-off-by: Karl Wiberg <kha@treskal.com>

---

 stgit/run.py |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)


diff --git a/stgit/run.py b/stgit/run.py
index 7493ed3..311d12f 100644
--- a/stgit/run.py
+++ b/stgit/run.py
@@ -110,7 +110,9 @@ class Run:
                                  stdin = subprocess.PIPE,
                                  stdout = subprocess.PIPE,
                                  stderr = subprocess.PIPE)
-            outdata, errdata = p.communicate(self.__indata)
+            if self.__indata:
+                p.stdin.write(self.__indata)
+            outdata, errdata = p.communicate()
             self.exitcode = p.returncode
         except OSError, e:
             raise self.exc('%s failed: %s' % (self.__cmd[0], e))

Re: [PATCH] Work around performance bug in subprocess.Popen.communicate()

[Catalin Marinas]

2009/7/31 Karl Wiberg <kha@virtutech.com>:
> @@ -110,7 +110,9 @@ class Run:
>                                  stdin = subprocess.PIPE,
>                                  stdout = subprocess.PIPE,
>                                  stderr = subprocess.PIPE)
> -            outdata, errdata = p.communicate(self.__indata)
> +            if self.__indata:
> +                p.stdin.write(self.__indata)
> +            outdata, errdata = p.communicate()
>             self.exitcode = p.returncode
>         except OSError, e:
>             raise self.exc('%s failed: %s' % (self.__cmd[0], e))

But can this not lead to a deadlock if the __indata is big? The stdout
of the created process is only processed once the whole __indata is
written. I thought communicate() was created to avoid this issue.

-- 
Catalin

Re: [PATCH] Work around performance bug in subprocess.Popen.communicate()

[Karl Wiberg]

On 2009-07-31 12:27:53 +0100, Catalin Marinas wrote:

> But can this not lead to a deadlock if the __indata is big? The
> stdout of the created process is only processed once the whole
> __indata is written. I thought communicate() was created to avoid
> this issue.

I don't think there's a problem. write() isn't supposed to have a
limit on the amount of data it will accept in one call, as far as I'm
aware. Plus, it works just fine with Erik's test case---which in my
case was about 7 MB. If it can handle 7 MB, I doubt there's a limit
we'll hit anytime soon.

Oh, and we still call communicate()---we just don't pass it any
additional bytes to write to stdin.

-- 
Karl Wiberg, Virtutech

Re: [PATCH] Work around performance bug in subprocess.Popen.communicate()

[Catalin Marinas]

2009/8/4 Karl Wiberg <kha@virtutech.com>:
> On 2009-07-31 12:27:53 +0100, Catalin Marinas wrote:
>
>> But can this not lead to a deadlock if the __indata is big? The
>> stdout of the created process is only processed once the whole
>> __indata is written. I thought communicate() was created to avoid
>> this issue.
>
> I don't think there's a problem. write() isn't supposed to have a
> limit on the amount of data it will accept in one call, as far as I'm
> aware. Plus, it works just fine with Erik's test case---which in my
> case was about 7 MB. If it can handle 7 MB, I doubt there's a limit
> we'll hit anytime soon.

write() itself doesn't have a limit, it's mainly what the application
receiving the data can handle. In the Git case, I think it takes all
the input as it isn't a filtering tool (things may be different with
tools like sed etc.).

> Oh, and we still call communicate()---we just don't pass it any
> additional bytes to write to stdin.

Yes, but if write() is blocked, communicate() won't be called.

Since we are only using Git, I'll merge this patch (and maybe add a comment).

-- 
Catalin

Re: [PATCH] Work around performance bug in subprocess.Popen.communicate()

[Karl Wiberg]

On 2009-08-13 23:18:59 +0100, Catalin Marinas wrote:

> 2009/8/4 Karl Wiberg <kha@virtutech.com>:
>
> > On 2009-07-31 12:27:53 +0100, Catalin Marinas wrote:
> >
> > > But can this not lead to a deadlock if the __indata is big? The
> > > stdout of the created process is only processed once the whole
> > > __indata is written. I thought communicate() was created to
> > > avoid this issue.
> >
> > I don't think there's a problem. write() isn't supposed to have a
> > limit on the amount of data it will accept in one call, as far as
> > I'm aware. Plus, it works just fine with Erik's test case---which
> > in my case was about 7 MB. If it can handle 7 MB, I doubt there's
> > a limit we'll hit anytime soon.
>
> write() itself doesn't have a limit, it's mainly what the
> application receiving the data can handle. In the Git case, I think
> it takes all the input as it isn't a filtering tool (things may be
> different with tools like sed etc.).
>
> > Oh, and we still call communicate()---we just don't pass it any
> > additional bytes to write to stdin.
>
> Yes, but if write() is blocked, communicate() won't be called.

Ah, so that's what you were worrying about. Yeah, if the process we're
writing to was going to block us (== not read input) for long periods
of time, we'd have to handle that case. And if it was going to decide
to stop reading input half-way through and give us a sigpipe, we'd
have to handle that. But ...

> Since we are only using Git, I'll merge this patch

Exactly. We really don't expect git to do any of those things to us.
We know it reads all available input before producing the output.
(Well, almost always. I have patches in my experimental branch that
use git cat-file --batch and git diff-tree --stdin, where there can be
many rounds of talking back and forth, but when driving those I'm
being very careful to not deadlock.)

> (and maybe add a comment).

Sure.

-- 
Karl Wiberg, Virtutech

Re: [PATCH] Work around performance bug in subprocess.Popen.communicate()

[Erik Sandberg]

On Fri, Aug 14, 2009 at 8:21 AM, Karl Wiberg<kha@virtutech.com> wrote:
> On 2009-08-13 23:18:59 +0100, Catalin Marinas wrote:
>
>> Since we are only using Git, I'll merge this patch
>> (and maybe add a comment).
>
> Sure.

Wouldn't it be better to only use the write() workaround when
sys.version indicates that the Python bug may be present, and use
communicate() properly otherwise?

Erik

Re: [PATCH] Work around performance bug in subprocess.Popen.communicate()

[Karl Wiberg]

On 2009-08-14 09:26:06 +0200, Erik Sandberg wrote:

> Wouldn't it be better to only use the write() workaround when
> sys.version indicates that the Python bug may be present, and use
> communicate() properly otherwise?

In my opinion: no. The workaround has no negative effect (since we
know that git is well-behaved). Making it conditional would only make
the code more complex, without gaining us anything.

-- 
Karl Wiberg, Virtutech