Merge lp:~jameinel/bzr/2.0.4-494406-serve-memory-leak into lp:bzr

[John A Meinel]

[John A Meinel (jameinel)]

I managed to track down the memory leak on server-side operations to bzrlib.bencode.Encoder.

It turns out that Encoder was using '__del__' as its cleanup function, when it should have been '__dealloc__'. I don't know if __del__ ever gets called for Pyrex objects, but I do know that __dealloc__ always gets called. (And doesn't cause problems with GC cycles, as it gets put into the tp_dealloc slot.)

Anyway, this one-line patch gets rid of the memory leak as near as I can tell. (At least cycling on 'bzr commit' no longer leaks memory.)

It could also have a fair impact on codehostings 'lp-serve'. As that would be encoding a *lot* of bytes to send over the wire. This memory was also "dark" memory, in that it was allocated directly using 'malloc()' and thus not seen by Meliae. I'm not positive, but I think for every byte that 'bzr lp-serve' would send over the wire, it would get wrapped by a 'bencode' operation, which means that every byte sent out gets permanently leaked in memory. (Maybe we don't serve bulk data through a bencode wrapper.) At least, all of the response tuples are certainly encoded.

It doesn't affect my 'memory reduction' tests that I used for 2.1.0b3, because those were operating locally.

[John A Meinel (jameinel)]

This was meant to be proposed for 2.0
https://code.edge.launchpad.net/~jameinel/bzr/2.0.4-494406-serve-memory-leak/+merge/16369

Please review it there.

=== modified file 'NEWS'
--- NEWS	2009-12-18 09:05:13 +0000
+++ NEWS	2009-12-18 22:32:31 +0000
@@ -50,7 +50,7 @@
 bzr 2.0.4 (not released yet)
 ############################
 
-:Codename: template
+:Codename:
 :2.0.4: ???
 
 Compatibility Breaks
@@ -62,6 +62,17 @@
 Bug Fixes
 *********
 
+* ``bzr export dir`` now requests all file content as a record stream,
+  rather than requsting the file content one file-at-a-time. This can make
+  exporting over the network significantly faster (54min => 9min in one
+  case). (John Arbash Meinel, #343218)
+
+* ``bzr serve`` no longer slowly leaks memory. The compiled
+  ``bzrlib.bencode.Encoder()`` class was using ``__del__`` to cleanup and
+  free resources, and it should have been using ``__dealloc__``.
+  This will likely have an impact on any other process that is serving for
+  an extended period of time.  (John Arbash Meinel, #494406)
+
 Improvements
 ************
 
 
=== modified file 'bzrlib/_bencode_pyx.pyx'
--- bzrlib/_bencode_pyx.pyx	2009-10-15 18:28:14 +0000
+++ bzrlib/_bencode_pyx.pyx	2009-12-18 22:32:31 +0000
@@ -268,7 +268,7 @@
         self.maxsize = maxsize
         self.tail = p
 
-    def __del__(self):
+    def __dealloc__(self):
         free(self.buffer)
         self.buffer = NULL
         self.maxsize = 0
 
=== modified file 'bzrlib/export/dir_exporter.py'
--- bzrlib/export/dir_exporter.py	2009-07-29 13:46:55 +0000
+++ bzrlib/export/dir_exporter.py	2009-12-18 22:32:31 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005 Canonical Ltd
+# Copyright (C) 2005, 2009 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -52,21 +52,17 @@
                 raise errors.BzrError("Can't export tree to non-empty directory.")
         else:
             raise
+    # Iterate everything, building up the files we will want to export, and
+    # creating the directories and symlinks that we need.
+    # This tracks (file_id, (destination_path, executable))
+    # This matches the api that tree.iter_files_bytes() wants
+    # Note in the case of revision trees, this does trigger a double inventory
+    # lookup, hopefully it isn't too expensive.
+    to_fetch = []
     for dp, ie in _export_iter_entries(tree, subdir):
         fullpath = osutils.pathjoin(dest, dp)
         if ie.kind == "file":
-            if filtered:
-                chunks = tree.get_file_lines(ie.file_id)
-                filters = tree._content_filter_stack(dp)
-                context = ContentFilterContext(dp, tree, ie)
-                contents = filtered_output_bytes(chunks, filters, context)
-                content = ''.join(contents)
-                fileobj = StringIO.StringIO(content)
-            else:
-                fileobj = tree.get_file(ie.file_id)
-            osutils.pumpfile(fileobj, file(fullpath, 'wb'))
-            if tree.is_executable(ie.file_id):
-                os.chmod(fullpath, 0755)
+            to_fetch.append((ie.file_id, (dp, tree.is_executable(ie.file_id))))
         elif ie.kind == "directory":
             os.mkdir(fullpath)
         elif ie.kind == "symlink":
@@ -80,3 +76,21 @@
         else:
             raise errors.BzrError("don't know how to export {%s} of kind %r" %
                (ie.file_id, ie.kind))
+    # The data returned here can be in any order, but we've already created all
+    # the directories
+    flags = os.O_CREAT | os.O_TRUNC | os.O_WRONLY | getattr(os, 'O_BINARY', 0)
+    for (relpath, executable), chunks in tree.iter_files_bytes(to_fetch):
+        if filtered:
+            filters = tree._content_filter_stack(relpath)
+            context = ContentFilterContext(relpath, tree, ie)
+            chunks = filtered_output_bytes(chunks, filters, context)
+        fullpath = osutils.pathjoin(dest, relpath)
+        # We set the mode and let the umask sort out the file info
+        mode = 0666
+        if executable:
+            mode = 0777
+        out = os.fdopen(os.open(fullpath, flags, mode), 'wb')
+        try:
+            out.writelines(chunks)
+        finally:
+            out.close()
 
=== modified file 'doc/en/user-guide/introducing_bazaar.txt'