Re: [RFC/PATCH] Handling PROPFIND in mod_dav_svn

[Stefan Fuhrmann <st...@apache.org>]

On 13.01.2016 15:33, Evgeny Kotkov wrote:
> Stefan Fuhrmann <st...@apache.org> writes:
> If I replace apr_palloc() with malloc() / free() and patch mod_dav to avoid
> creating the propdb->p subpools, I still see inadequate memory consumption.
>
> The environment is close to the one reported by the user, and preparing a
> 4.5 MB response causes the heap commit size to slowly increase up to 70 MB.
> This is a lot, considering a possible amount of parallel requests and *can*
> cause problems in any reasonable setup.  It could as well be much worse with
> other possible environments and data.

Sure and I don't deny neither the existence nor the relevance
of the problem.  All I want to point out that there are two
levels of severity here - one easily consuming 10s of GB per
request and one using 100 MB or so for a directory with no user
properties at its entries.

The first is being addressed by my patches, the latter is basically
what you got with 1.6-style caching anyway and what you are
trying to solve.

>> Does the patch already include the improvements you suggested above?
>> Is it feasible to #ifdef the usage of that code?  I think if we have
>> an easy and clean opt-in / opt-out, people could be more accepting of
>> that workaround.
>
> The patch doesn't include these improvements.  I can drive the patch up to
> a committable state, commit it, and we can enhance these aspects later in
> trunk.  I attached an updated version of the patch without the regression
> tests that were committed in r1724103.

Having the option to reduce the server load and network
traffic would certainly be a plus.  plugins.svn.wordpress.org
is now closing it at 60k entries in the repo root.

> I don't really like the idea of surrounding this code with #ifdef-s, since
> I can't see a reason to conditionally turn it off, and not enabling it by
> default would leave us with undertested code or code that's never executed.

The point of the #ifdef would be an opt-out for the time the
problem got fixed on the mod_dav side.  I agree that making it
opt-in would be problematic on multiple levels.  Also, if it's
only a handful of #ifdef sections, then the change is reasonably
isolated from the rest, i.e. has a reduced perceived risk.

I still think we need to fix eventually fix mod_dav.  In the
long run, that should always be cheaper than maintaining a fork.

-- Stefan^2.