Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Date: Sun, 28 Apr 2002 11:49:39 -0700
From: Brian Pane <bpane@pacbell.net>
Subject: Re: cvs commit: httpd-2.0/server/mpm/worker worker.c fdqueue.c
 fdqueue.h
To: dev@httpd.apache.org
Message-id: <3CCC4443.4080102@pacbell.net>
MIME-version: 1.0
Content-type: text/plain; charset=us-ascii; format=flowed
Content-transfer-encoding: 7BIT
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.9) Gecko/20020310
References: <20020428014500.95524.qmail@icarus.apache.org>
 <20020427193051.G31982@apache.org> <20020427201711.U14899@clove.org>
 <004701c1eec0$8a0269b0$6701a8c0@sashimi>

Bill Stoddard wrote:

>
>>On Sat, Apr 27, 2002 at 07:30:51PM -0700, Justin Erenkrantz wrote:
>>
>>>>  +    qi = apr_palloc(pool, sizeof(*qi));
>>>>  +    memset(qi, 0, sizeof(*qi));
>>>>
>>>As we said, if you are concerned about the performance aspect
>>>of apr_pcalloc, then we should fix apr_pcalloc NOT attempt to
>>>work around its inefficiencies by pointedly not using it.
>>>
>>>If/when Cliff (or someone else?) commits the change to apr_pcalloc,
>>>this chunk would be magically changed along with everything else if
>>>you simply called apr_pcalloc in the first place.
>>>
>>We don't have a consensus on this, and I'm ambivalent about making the
>>p_calloc macro. If we do come up with a consensus than it can change.
>>Until then this is more correct than using p_calloc.
>>
>
>I have no problem with using apr_palloc()/memset() in place of apr_pcalloc().
>

I'm +1 on turning pcalloc into a macro,
    -0 for using pcalloc in ap_queue_info_init(),
    -0.5 (non-veto) for using palloc/memset in ap_queue_info_init()

Rationale:
* Changing pcalloc to a macro will give us a free optimization
  throughout the entire code base.

* In ap_queue_info_init(), we create a struct with 7 fields and
  immediately set 4 of them to nonzero values.  Using pcalloc, or
  any other form of block zero-fill, isn't a big win.

* The whole point of the palloc/memset idiom is that
  it's a more awkward syntax than pcalloc, but justified
  for the sake of performance optimization.  But there's
  no point in optimizing ap_queue_info_init().  The function
  gets called once per child proc, at startup, and no matter
  what technique you use to zero-fill the newly created struct,
  it's going to account for far too little of the total run
  time to matter at all.  There's plenty of code within
  fdqueue.c that could benefit from performance optimization,
  but none of it is in this function.

>>>>  +    rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
>>>>  +    if (rv != APR_SUCCESS) {
>>>>  +        return rv;
>>>>  +    }
>>>>  +    return APR_SUCCESS;
>>>>  +}
>>>>
>>>As I said before, simply "return rv;" works here.
>>>
>>Yeah, but this is much more readable.
>>
>
>I disagree. I would rather see "return rv".
>

I'd rather see "return rv" or "return apr_thread_mutex_unlock".
Both are more readable (and faster) than the current code with
the if-statement.

--Brian