On 13.01.2016 15:33, Evgeny Kotkov wrote:
> Stefan Fuhrmann <stefan2_at_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.
Received on 2016-01-15 15:35:19 CET