[PATCH] Reduce memory footprint in mod_dav's property code
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 1 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(>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
}
]]]
Re: [PATCH] Reduce memory footprint in mod_dav's property code
You can also have a look at
https://bz.apache.org/bugzilla/show_bug.cgi?id=48130
Le 22/12/2015 01:13, Stefan Fuhrmann a écrit :
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 1 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(>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
}
]]]
