Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
MIME-Version: 1.0
In-Reply-To: <2791C826-8CFB-4DC9-B6AF-566DF625B154@sharp.fm>
References: 
 <CAKQ1sVMoxQe=8iAQ=oUSRJhrpy0o8RpJ=+u_CXecw8UbTj_NOg@mail.gmail.com>
	<2791C826-8CFB-4DC9-B6AF-566DF625B154@sharp.fm>
Date: Tue, 6 Oct 2015 13:02:11 +0200
Message-ID: 
 <CAKQ1sVOCaz2BuzAAm399-Kbxz6WsncDB+uasAgq58W6e_b4+aQ@mail.gmail.com>
Subject: Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/
 modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/
 server/mpm/simple/
From: Yann Ylavic <ylavic.dev@gmail.com>
To: dev@httpd.apache.org
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Tue, Oct 6, 2015 at 12:15 PM, Graham Leggett <minfrin@sharp.fm> wrote:
> On 06 Oct 2015, at 1:39 AM, Yann Ylavic <ylavic.dev@gmail.com> wrote:
>
>>> +
>>> +    /** Buffered data associated with the current filter. */
>>> +    apr_bucket_brigade *bb;
>>> +
>>> +    /** Dedicated pool to use for deferred writes. */
>>> +    apr_pool_t *deferred_pool;
>>
>> Could we make these opaque, eg:
[]
>>
>> This would prevent them from being mangled anywhere (they are
>> internals to util_filter.c anyway).
>
> There weren=E2=80=99t originally internal to util_filter.c, it=E2=80=99s =
only subsequently turned out that way. Doing that comes at a cost of a furt=
her 8 bytes per filter per request through, which starts adding up.

8 or a few more bytes is a concern, really?

>
> I would rather finish the filter suspension work before doing any optimis=
ing like this, it=E2=80=99s too soon at this stage.

Fair enough.

>
>> It could also possibly avoid walking the brigade for every
>> ap_filter_reinstate_brigade() call=E2=80=A6
>
> Unfortunately you can=E2=80=99t avoid walking the brigade in the ap_filte=
r_reinstate_brigade(), as this is the flow control logic that keeps the ser=
ver safe from consuming too much data.

Well, there are only invariants here, the loop always computes the
same values for the buffered_bb or am I missing something?

>
>> Also it seems that we leak the hash iterator here (on c->pool).
>
> We don=E2=80=99t, when you create an iterator with a NULL pool it uses an=
 iterator internal to the hash. If we passed c->pool to apr_hash_first(), w=
e would leak.

Correct, R=C3=BCdiger spotted it already, my bad.

>
>> Shouldn't we call filters_cleanup() from ap_remove_output_filter() too?
>
> I deliberately decided not to.
>
> The majority of calls to ap_remove_output_filter() are made when filters =
step out the way before data starts flowing, and if we tried to run the cle=
anup we=E2=80=99d be performing an unnecessary apr_hash_set() on every invo=
cation before the filter was ever added to the hash.
>
> The point at which it would make a difference is after the EOS bucket is =
handled, but at this point it doesn=E2=80=99t matter, all we=E2=80=99re doi=
ng is saving one NULL check in the MPM on a loop with a handful of iteratio=
ns, it=E2=80=99s not worth the expense.


>
>> Why not simply use:
>>   key =3D apr_pmemdup(pool, &f, sizeof f);
>>   apr_hash_set(f->c->filters, &key, sizeof key, f)
>> here, and:
>>   ap_filter_t *f =3D data;
>>   apr_hash_set(f->c->filters, &f, sizeof f, NULL);
>> in filters_cleanup() above?
>
> We could but it=E2=80=99s more expensive. The memdup part is replaced by =
simply assigning a pointer.

Not sure about the overhead, memcpy is optimized for small sizes anyway.
At least both "sizeof(ap_filter_t **)" here and in filters_cleanup()
should be replaced by "sizeof(ap_filter_t *)" which is syntactically
more correct (even if both are equal in C).

>
>>> +    *flush_upto =3D NULL;
>>> +
>>> +    bytes_in_brigade =3D 0;
>>> +    non_file_bytes_in_brigade =3D 0;
>>> +    eor_buckets_in_brigade =3D 0;
>>> +    morphing_bucket_in_brigade =3D 0;
>>
>> Per the ealier suggestion to make ap_filter_data opaque, these could
>> be part of the struct (and reentrant).
>> We could then PREPEND after the loop below, and avoid potentially to
>> walk the same buckets each time.
>
> Alas as described above that=E2=80=99s not possible, as it breaks the flo=
w control mechanism that prevents the server generating too much data.

Unless what's in buffered_bb is opaque (immutable between calls)...

Regards,
Yann.