[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Bug 62590] performance drop after moving from apache 2.2 to apache 2.4

Just for interest ran my h2 load tests on trunk/2.4.x unpatched and with commenting ERR_clear_error(); in ssl_engine_io.c

On my Mac I see no differences, really. trunk used openssl 1.0.2o and 2.4.x had 1.0.2l:

> h2load -i gen/load-urls-1.txt -n 200000 -t 4 -m 100 -c <clients>
    raw, 128 clients: 33940.36 req/s
patched, 128 clients: 33699.89 req/s

    raw, 128 clients: 33417.12 req/s
patched, 128 clients: 34071.83 req/s

(just some runs, nothing with any statistical relevance)

Caveat: this does not necessarily say anything about the http/1.1 performance. h2 buffers its writes into he ssl output filters more than h1 does, I think.

Cheers, Stefan

> Am 29.08.2018 um 09:55 schrieb Ruediger Pluem <rpluem@xxxxxxxxxx>:
> On 08/29/2018 09:37 AM, Christophe Jaillet wrote:
>> Pure speculation:
>> Actually we ERR_clear_error unconditionally so that in case of error, we can safely call SSL_get_error.
>> Doc says:
>> <<<<<<<<<< >>>>>>>>>>
>> ERR_get_error() returns the earliest error code from the thread's error queue and removes the entry. This function can be called repeatedly until there are no more error codes to return.
>> ERR_peek_error() returns the earliest error code from the thread's error queue without modifying it.
>> ERR_peek_last_error() returns the latest error code from the thread's error queue without modifying it.
>> <<<<<<<<<< >>>>>>>>>>
>> Couldn't we avoid the ERR_clear_error call (which looks expensive according to the thread), and:
>>   - loop on SSL_get_error to empty the error queue, in case an error occurred and we want la latest one?
>> or
>>   - do a ERR_peek_last_error() / ERR_clear_error() in the error handling path (if we really care about clearing the error queue)
>> IMHO, these 2 solutions would do the same as the current code, without requiring ERR_clear_error in the normal case.
> Unfortunately it isn't that easy, because SSL_get_error does not only look at the error stack but also at the return
> code from a read / write call passed to it. It quickly breaks out with two possible error return values if something
> is on the error stack and this what caused the addition of the ERR_clear_error call. We had the situation that another
> consumer of openssl in the code base left an error unhandled on the stack and thus caused SSL_get_error to draw the
> wrong conclusions. While we could argue the error is in the other part of the code and the other consumer of openssl
> should clear up the error stack correctly I don't think this is the complete solution, because
> 1. We should not fail if a another consumer is bogus. We should be prepared for others to have bugs regarding this.
> 2. As the error handling is a stack I think it is a valid use case for another consumer of openssl to check for the
>   error later and not directly like e.g. with errno.
> I am not sure yet if it is not SSL_get_error that makes wrong assumptions about the error stack, but that would be
> something hard to solve for us.
> Regards
> Rüdiger