Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Message-ID: <5678958E.20608@apache.org>
Date: Tue, 22 Dec 2015 01:13:02 +0100
From: Stefan Fuhrmann <stefan2@apache.org>
Organization: Apache Software Foundation
User-Agent: Mozilla/5.0 (X11; Linux x86_64;
 rv:24.0) Gecko/20100101 Thunderbird/24.6.0
MIME-Version: 1.0
To: dev@httpd.apache.org
Subject: [PATCH] Reduce memory footprint in mod_dav's property code
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi,

I stumbled over this while investigation an OOM report from a
Subversion user [1].  Due to unfortunate circumstances [2], I've
seen directory listings with a few 10000 entries eat 10s of GB
of RAM.  Those circumstances are being addressed but the root
cause is in mod_dav and the patch below fixes it.

The problem is that dav_open_propdb creates a pool that
dav_close_propdb can't destroy.  So, for every property, at least
8 KB of memory will be allocated.  If we simply use the parent
pool as is, only a few 100 bytes will be used.

Moreover, if the APR allocator should return a larger block once
in a while, that block will get used up nicely before more memory
is allocated.  So with this patch, the worst case will still use
only about to 1/10th of the current best case for large collections.

-- Stefan^2.

[1] 
http://mail-archives.apache.org/mod_mbox/subversion-users/201512.mbox/%3C1CEE115D02633942A40D49D447DCF46E432F3A32%40SD01CFMM0202.OMEGA.DCE-EIR.NET%3E
[2] 
http://mail-archives.apache.org/mod_mbox/subversion-dev/201512.mbox/%3C5676A54F.9040609%40apache.org%3E


[[[
Fix a pool handling asymmetry in mod_dav that caused inefficient
memory usage with collection properties.

If we can't clear nor destroy the local pool in dav_propdb, there
is no point in creating it as a sub-pool of its parent.

* modules/dav/main/props.c
   (DAV_PROPDB_LOCAL_POOL): New compile-time switch replacing the
                            plain "#if 0" in dav_close_propdb.  It
                            ensures consistent behaviour between
                            open and close in the future.
   (dav_open_propdb): If we can't clear the pool, we may as well use
                      its parent directly.
   (dav_close_propdb): Use the new define.
]]]

[[[
Index: modules/dav/main/props.c
===================================================================
--- modules/dav/main/props.c	(revision 1721073)
+++ modules/dav/main/props.c	(working copy)
@@ -167,6 +167,16 @@

  #define DAV_EMPTY_VALUE                "\0"    /* TWO null terms */

+/*
+** Currently, mod_dav's pool usage doesn't allow clearing the pool
+** at dav_propdb.p . Therefore, we won't create a sub-pool for it
+** and use the request's pool directly instead.
+**
+** Once the pool usage issue has been fixed, set this to 1 for optimal
+** memory usage.
+*/
+#define DAV_PROPDB_LOCAL_POOL          0
+
  struct dav_propdb {
      apr_pool_t *p;                /* the pool we should use */
      request_rec *r;               /* the request record */
@@ -537,7 +547,11 @@
  #endif

      propdb->r = r;
+#if DAV_PROPDB_LOCAL_POOL
      apr_pool_create(&propdb->p, r->pool);
+#else
+    propdb->p = r->pool;
+#endif
      propdb->resource = resource;
      propdb->ns_xlate = ns_xlate;

@@ -562,8 +576,7 @@
          (*propdb->db_hooks->close)(propdb->db);
      }

-    /* Currently, mod_dav's pool usage doesn't allow clearing this pool. */
-#if 0
+#if DAV_PROPDB_LOCAL_POOL
      apr_pool_destroy(propdb->p);
  #endif
  }
]]]