{"thread":{"id":"20315","subject":"[PATCH] Work around performance bug in subprocess.Popen.communicate()","startedAt":"2009-07-31T09:36:32Z","lastAt":"2009-08-14T08:12:20Z","messageCount":7,"participants":["Karl Wiberg","Catalin Marinas","Erik Sandberg"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"119244","messageId":"20090731093632.7018.24435.stgit@october.e.vtech","threadId":"20315","inReplyTo":null,"subject":"[PATCH] Work around performance bug in subprocess.Popen.communicate()","fromName":"Karl Wiberg","fromEmail":"kha@virtutech.com","sentAt":"2009-07-31T09:36:32Z","receivedAt":"2009-07-31T09:36:32Z","isPatch":true,"sender":{"key":"kha@virtutech.com","avatar":null},"body":"From: Karl Wiberg \n\nIn Python 2.4 (specifically, I tested with 2.4.6 on Ubuntu 9.04),\nsubprocess.Popen.communicate() seems to take time proportional to the\nsquare of the size of the indata, which makes it ridiculously\nexpensive to write stack log entries when the diffs are large. Work\naround the bug by calling subprocess.Popen.stdin.write() manually\ninstead of letting communicate() handle the indata.\n\nThe performance bug has been fixed in Python 2.6 (I tested with\n2.6.2), so with that version this workaround doesn't affect the run\ntime. I haven't tested with Python 2.5.\n\nThis fixes bug 13319.\n\nSigned-off-by: Karl Wiberg \n\n---\n\n stgit/run.py | 4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\n\ndiff --git a/stgit/run.py b/stgit/run.py\nindex 7493ed3..311d12f 100644\n--- a/stgit/run.py\n+++ b/stgit/run.py\n@@ -110,7 +110,9 @@ class Run:\n stdin = subprocess.PIPE,\n stdout = subprocess.PIPE,\n stderr = subprocess.PIPE)\n- outdata, errdata = p.communicate(self.__indata)\n+ if self.__indata:\n+ p.stdin.write(self.__indata)\n+ outdata, errdata = p.communicate()\n self.exitcode = p.returncode\n except OSError, e:\n raise self.exc('%s failed: %s' % (self.__cmd[0], e))\n"},{"id":"119255","messageId":"b0943d9e0907310427r7e59aa5fi5e5ff5e3f3c03819@mail.gmail.com","threadId":"20315","inReplyTo":"20090731093632.7018.24435.stgit@october.e.vtech","subject":"Re: [PATCH] Work around performance bug in subprocess.Popen.communicate()","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2009-07-31T11:27:53Z","receivedAt":"2009-07-31T11:27:53Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2009/7/31 Karl Wiberg :\n> @@ -110,7 +110,9 @@ class Run:\n>                                  stdin = subprocess.PIPE,\n>                                  stdout = subprocess.PIPE,\n>                                  stderr = subprocess.PIPE)\n> -            outdata, errdata = p.communicate(self.__indata)\n> +            if self.__indata:\n> +                p.stdin.write(self.__indata)\n> +            outdata, errdata = p.communicate()\n>             self.exitcode = p.returncode\n>         except OSError, e:\n>             raise self.exc('%s failed: %s' % (self.__cmd[0], e))\n\nBut can this not lead to a deadlock if the __indata is big? The stdout\nof the created process is only processed once the whole __indata is\nwritten. I thought communicate() was created to avoid this issue.\n\n-- \nCatalin\n"},{"id":"119485","messageId":"20090804085125.GB27452@lux.e.vtech","threadId":"20315","inReplyTo":"b0943d9e0907310427r7e59aa5fi5e5ff5e3f3c03819@mail.gmail.com","subject":"Re: [PATCH] Work around performance bug in subprocess.Popen.communicate()","fromName":"Karl Wiberg","fromEmail":"kha@virtutech.com","sentAt":"2009-08-04T08:51:25Z","receivedAt":"2009-08-04T08:51:25Z","isPatch":true,"sender":{"key":"kha@virtutech.com","avatar":null},"body":"On 2009-07-31 12:27:53 +0100, Catalin Marinas wrote:\n\n> But can this not lead to a deadlock if the __indata is big? The\n> stdout of the created process is only processed once the whole\n> __indata is written. I thought communicate() was created to avoid\n> this issue.\n\nI don't think there's a problem. write() isn't supposed to have a\nlimit on the amount of data it will accept in one call, as far as I'm\naware. Plus, it works just fine with Erik's test case---which in my\ncase was about 7 MB. If it can handle 7 MB, I doubt there's a limit\nwe'll hit anytime soon.\n\nOh, and we still call communicate()---we just don't pass it any\nadditional bytes to write to stdin.\n\n-- \nKarl Wiberg, Virtutech\n"},{"id":"120595","messageId":"b0943d9e0908131518i3ac18331leb4c0c76313b0780@mail.gmail.com","threadId":"20315","inReplyTo":"20090804085125.GB27452@lux.e.vtech","subject":"Re: [PATCH] Work around performance bug in subprocess.Popen.communicate()","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2009-08-13T22:18:59Z","receivedAt":"2009-08-13T22:18:59Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2009/8/4 Karl Wiberg :\n> On 2009-07-31 12:27:53 +0100, Catalin Marinas wrote:\n>\n>> But can this not lead to a deadlock if the __indata is big? The\n>> stdout of the created process is only processed once the whole\n>> __indata is written. I thought communicate() was created to avoid\n>> this issue.\n>\n> I don't think there's a problem. write() isn't supposed to have a\n> limit on the amount of data it will accept in one call, as far as I'm\n> aware. Plus, it works just fine with Erik's test case---which in my\n> case was about 7 MB. If it can handle 7 MB, I doubt there's a limit\n> we'll hit anytime soon.\n\nwrite() itself doesn't have a limit, it's mainly what the application\nreceiving the data can handle. In the Git case, I think it takes all\nthe input as it isn't a filtering tool (things may be different with\ntools like sed etc.).\n\n> Oh, and we still call communicate()---we just don't pass it any\n> additional bytes to write to stdin.\n\nYes, but if write() is blocked, communicate() won't be called.\n\nSince we are only using Git, I'll merge this patch (and maybe add a comment).\n\n-- \nCatalin\n"},{"id":"120607","messageId":"20090814062149.GA23179@lux.e.vtech","threadId":"20315","inReplyTo":"b0943d9e0908131518i3ac18331leb4c0c76313b0780@mail.gmail.com","subject":"Re: [PATCH] Work around performance bug in subprocess.Popen.communicate()","fromName":"Karl Wiberg","fromEmail":"kha@virtutech.com","sentAt":"2009-08-14T06:21:49Z","receivedAt":"2009-08-14T06:21:49Z","isPatch":true,"sender":{"key":"kha@virtutech.com","avatar":null},"body":"On 2009-08-13 23:18:59 +0100, Catalin Marinas wrote:\n\n> 2009/8/4 Karl Wiberg :\n>\n> > On 2009-07-31 12:27:53 +0100, Catalin Marinas wrote:\n> >\n> > > But can this not lead to a deadlock if the __indata is big? The\n> > > stdout of the created process is only processed once the whole\n> > > __indata is written. I thought communicate() was created to\n> > > avoid this issue.\n> >\n> > I don't think there's a problem. write() isn't supposed to have a\n> > limit on the amount of data it will accept in one call, as far as\n> > I'm aware. Plus, it works just fine with Erik's test case---which\n> > in my case was about 7 MB. If it can handle 7 MB, I doubt there's\n> > a limit we'll hit anytime soon.\n>\n> write() itself doesn't have a limit, it's mainly what the\n> application receiving the data can handle. In the Git case, I think\n> it takes all the input as it isn't a filtering tool (things may be\n> different with tools like sed etc.).\n>\n> > Oh, and we still call communicate()---we just don't pass it any\n> > additional bytes to write to stdin.\n>\n> Yes, but if write() is blocked, communicate() won't be called.\n\nAh, so that's what you were worrying about. Yeah, if the process we're\nwriting to was going to block us (== not read input) for long periods\nof time, we'd have to handle that case. And if it was going to decide\nto stop reading input half-way through and give us a sigpipe, we'd\nhave to handle that. But ...\n\n> Since we are only using Git, I'll merge this patch\n\nExactly. We really don't expect git to do any of those things to us.\nWe know it reads all available input before producing the output.\n(Well, almost always. I have patches in my experimental branch that\nuse git cat-file --batch and git diff-tree --stdin, where there can be\nmany rounds of talking back and forth, but when driving those I'm\nbeing very careful to not deadlock.)\n\n> (and maybe add a comment).\n\nSure.\n\n-- \nKarl Wiberg, Virtutech\n"},{"id":"120620","messageId":"e87cdfda0908140026x21b8aad3x82417d4b1c8016f4@mail.gmail.com","threadId":"20315","inReplyTo":"20090814062149.GA23179@lux.e.vtech","subject":"Re: [PATCH] Work around performance bug in subprocess.Popen.communicate()","fromName":"Erik Sandberg","fromEmail":"mandolaerik@gmail.com","sentAt":"2009-08-14T07:26:06Z","receivedAt":"2009-08-14T07:26:06Z","isPatch":true,"sender":{"key":"mandolaerik@gmail.com","avatar":null},"body":"On Fri, Aug 14, 2009 at 8:21 AM, Karl Wiberg wrote:\n> On 2009-08-13 23:18:59 +0100, Catalin Marinas wrote:\n>\n>> Since we are only using Git, I'll merge this patch\n>> (and maybe add a comment).\n>\n> Sure.\n\nWouldn't it be better to only use the write() workaround when\nsys.version indicates that the Python bug may be present, and use\ncommunicate() properly otherwise?\n\nErik\n"},{"id":"120625","messageId":"20090814081220.GA24722@lux.e.vtech","threadId":"20315","inReplyTo":"e87cdfda0908140026x21b8aad3x82417d4b1c8016f4@mail.gmail.com","subject":"Re: [PATCH] Work around performance bug in subprocess.Popen.communicate()","fromName":"Karl Wiberg","fromEmail":"kha@virtutech.com","sentAt":"2009-08-14T08:12:20Z","receivedAt":"2009-08-14T08:12:20Z","isPatch":true,"sender":{"key":"kha@virtutech.com","avatar":null},"body":"On 2009-08-14 09:26:06 +0200, Erik Sandberg wrote:\n\n> Wouldn't it be better to only use the write() workaround when\n> sys.version indicates that the Python bug may be present, and use\n> communicate() properly otherwise?\n\nIn my opinion: no. The workaround has no negative effect (since we\nknow that git is well-behaved). Making it conditional would only make\nthe code more complex, without gaining us anything.\n\n-- \nKarl Wiberg, Virtutech\n"}]}