httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Brian Pane <bp...@pacbell.net>
Subject Re: [PATCH 2] Re: [PATCH] performance patch for mod_log_config
Date Mon, 10 Sep 2001 20:35:38 GMT
dean gaudet wrote:

> why is there a need for 15 entries?  if it's a multiprocess server then
> there's only a need for 1 or 2 entries.  if it's a multithreaded server
> then you need to lock the cache (which you're not doing :) 

The whole point of the design is to do the multithreaded cache
without locks.  Instead of trying to prevent a race condition,
the cache is designed to make the race condition safe: when two
readers or a reader and a writer collide, the threads end up
computing the exploded time redundantly.  The race condition
that's not safe, though, is when one thread tries to explode
a time_t representing 'now - num_entries' seconds ago at the
same time that another thread is updating that same cache
element with the exploded value for the current time (the two
timestamps map to the same element in the ring buffer).  The
number of entries in the ring buffer thus determines how old
an input timestamp the cache can support and still provide
deterministic results: I used the 15-second value to provide
a large safety factor.

> isn't the real win in eliminating both the divisions required by the
> explode time functions and the divisions required by the printf?

That's what I used to think, but the profile data showed that localtime()
(which does most of the work in apr_explode_localtime()) is an order of
magnitude more expensive than the apr_snprintf() that formats the resulting
data.  Caching or otherwise optimizing the printf in the future wouldn't
hurt, but it's a more marginal win than caching the exploded time struct.

>   fwiw,
> tux does binary logging, and has an external tool to convert it to CLF...
> and the spec committee accepted that.  and tux also optimises the rfc822
> Date header by updating a cached copy once per second 

I was thinking doing something like that.  One of the alternate designs
that I considered would have had a thread running in the background would
update the time once a second, but I ended up picking the cache-on-demand
solution because it's more portable to threadless MPMs.  (I also 
contemplated
having the accept loop in the worker MPM wake up once a second to update the
cached time, but that was just too ugly. :-)

> also -- you could eliminate the seconds = t / APR_USECS_PER_SEC
> calculation (death to all divisions!  especially the 64-bit ones)...
> because this division already happens inside the explode function:
>
> when you miss the cache and do a full explode, then subtract the 
> resulting
> tm_usec off the input time and you've got an apr_time_t which lands
> exactly on a second boundary.  use that as your cache key, and you can
> then do ranged comparisons (t >= cache->t && t < cache->t +
> APR_USECS_PER_SEC). 

The one problem I can see with this is that removing the division means
that you have to scan through the table to find out whether you have a
cache hit or not--which probably negates the performance benefit of
removing the division.

--Brian




Mime
View raw message